Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for QoS in sample. #46

Merged

Conversation

DenisBiryukov91
Copy link
Contributor

Add support for QoS in sample (see eclipse-zenoh/zenoh#730)

@eclipse-zenoh-bot
Copy link
Contributor

@DenisBiryukov91 If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

Copy link
Member

@DariusIMP DariusIMP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Denis, I just left a couple of comments :)

@@ -16,8 +16,8 @@ package io.zenoh

import io.zenoh.keyexpr.intoKeyExpr
import io.zenoh.prelude.SampleKind
import io.zenoh.publication.CongestionControl
import io.zenoh.publication.Priority
import io.zenoh.prelude.CongestionControl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why moving sample kind and congestion control under prelude? I wonder because here for instance it's under publication, but it's true that it also appears under prelude (here). So what's the criteria, @p-avital may give us more insight

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mostly done to avoid circular dependencies between modules (yes, I know that kotlin/java can handle it). Also these types are really not something proper to publication only since they appear in other contexts (especially after this pr)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

Comment on lines 40 to 41
zenoh = { version = "0.11.0-dev", git = "https://github.com/DenisBiryukov91/zenoh.git", branch = "feature/priority-in-sample", default-features = false }
zenoh-ext = { version = "0.11.0-dev", git = "https://github.com/DenisBiryukov91/zenoh.git", branch = "feature/priority-in-sample", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for the changes to be under eclipse-zenoh's repository, in order to not make the packages dependent on a private repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines 20 to 60
/**
* Quality of service settings used to send zenoh message.
*/
class QoS internal constructor(internal val qos: Byte) {

/**
* Returns priority of the message.
*/
fun priority(): Priority {
return Priority.fromInt(getPriorityViaJNI(qos))
}

/**
* Returns congestion control setting of the message.
*/
fun congestionControl(): CongestionControl {
return CongestionControl.fromInt(getCongestionControlViaJNI(qos))
}

/**
* Returns express flag. If it is true, the message is not batched to reduce the latency.
*/
fun express(): Boolean {
return getExpressViaJNI(qos);
}

companion object {
/**
* Returns default QoS settings.
*/
fun default(): QoS {
return QoS(getDefaultViaJNI())
}

private external fun getDefaultViaJNI(): Byte
}

private external fun getPriorityViaJNI(_qos: Byte): Int
private external fun getCongestionControlViaJNI(_qos: Byte): Int
private external fun getExpressViaJNI(_qos:Byte): Boolean
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me suggest some changes here. So far all the communication through the JNI logic has been subjected to a delegation pattern, in which all the "ViaJNI" functions are contained within "JNI" objects on the 'jni' package in order to centralize that kind of logic there. So for the sake of sticking to that architectural design I'd do:

Suggested change
/**
* Quality of service settings used to send zenoh message.
*/
class QoS internal constructor(internal val qos: Byte) {
/**
* Returns priority of the message.
*/
fun priority(): Priority {
return Priority.fromInt(getPriorityViaJNI(qos))
}
/**
* Returns congestion control setting of the message.
*/
fun congestionControl(): CongestionControl {
return CongestionControl.fromInt(getCongestionControlViaJNI(qos))
}
/**
* Returns express flag. If it is true, the message is not batched to reduce the latency.
*/
fun express(): Boolean {
return getExpressViaJNI(qos);
}
companion object {
/**
* Returns default QoS settings.
*/
fun default(): QoS {
return QoS(getDefaultViaJNI())
}
private external fun getDefaultViaJNI(): Byte
}
private external fun getPriorityViaJNI(_qos: Byte): Int
private external fun getCongestionControlViaJNI(_qos: Byte): Int
private external fun getExpressViaJNI(_qos:Byte): Boolean
}
/**
* Quality of service settings used to send zenoh message.
*/
class QoS internal constructor(private val jniQoS: JNIQoS) {
internal constructor(qos: Byte): this(JNIQoS(qos))
/**
* Returns priority of the message.
*/
fun priority(): Priority = jniQoS.getPriority()
/**
* Returns congestion control setting of the message.
*/
fun congestionControl(): CongestionControl = jniQoS.getCongestionControl()
/**
* Returns express flag. If it is true, the message is not batched to reduce the latency.
*/
fun express(): Boolean = jniQoS.getExpress()
companion object {
/**
* Returns default QoS settings.
*/
fun default(): QoS {
return QoS(JNIQoS())
}
}
}

and where on the JNIQoS class contained within the jni package you'd have something like:

internal class JNIQoS internal constructor(private val qos: Byte) {

    internal constructor(): this(getDefaultQoSViaJNI())

    fun getExpress(): Boolean {
        return getExpressViaJNI(qos)
    }

    fun getCongestionControl(): CongestionControl {
        return CongestionControl.fromInt(getCongestionControlViaJNI(qos))
    }

    fun getPriority(): Priority {
        return Priority.fromInt(getPriorityViaJNI(qos))
    }

    private external fun getDefaultQoSViaJNI(): Byte
    private external fun getPriorityViaJNI(_qos: Byte): Int
    private external fun getCongestionControlViaJNI(_qos: Byte): Int
    private external fun getExpressViaJNI(_qos:Byte): Boolean
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @property attachment Optional [Attachment].
*/
class Sample(
val keyExpr: KeyExpr,
val value: Value,
val kind: SampleKind,
val timestamp: TimeStamp?,
val qos: QoS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can add a default value, in order to not needing to call QoS.default() wherever you create a new sample.

Suggested change
val qos: QoS,
val qos: QoS = QoS.default(),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issues is, there is no really default value of qos for sample, since it would depend on the context where sample is used (either in subscribe, or query.reply). It is to be addressed later in the sample rework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

Copy link
Member

@DariusIMP DariusIMP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

*/
fun express(): Boolean = jniQoS.getExpress()

internal fun jni(): JNIQoS = this.jniQoS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm back, just a bit of nitpicking, you don't need a function to return a private attribute the java way, just declare jniQoS as internal, and then instead of calling sample.qos.jni().qos call sample.qos.jniQoS.qos, just to avoid boilerplate code.
Also, can you add spaces between the functions and their kdoc comments? And newlines at the end of files that have them missing? That'd be nice. Besides that, LGTM.

@OlivierHecart OlivierHecart merged commit b884ce2 into eclipse-zenoh:main May 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants