Improve publish delivering to subscribers, part 1 #68
Conversation
…livering-to-subscribers
|
|
||
| abstract class QosMqttPublishOutMessageHandlerTest extends IntegrationServiceSpecification { | ||
| static { | ||
| } |
There was a problem hiding this comment.
Is the the static block required here?
There was a problem hiding this comment.
@crazyrokr the second part will be focused on tests and I will use it
There was a problem hiding this comment.
I advise against committing code for the future, because unclear intentions are confusing. If you need to add a static block in the future, it will be easier to do so than to remember a redundant empty block that needs to be removed if for some reason you don't need it.
| "mqtt.external.connection.subscription.id.available", | ||
| boolean.class, | ||
| MqttProperties.SUBSCRIPTION_IDENTIFIER_AVAILABLE_DEFAULT), | ||
| false), // not implemented |
There was a problem hiding this comment.
Is the comment required here?
There was a problem hiding this comment.
@crazyrokr it's description why I disabled it now
There was a problem hiding this comment.
The current comment doesn't clarify the original intent and is confusing. Please add more details to the comment or delete it.
crazyrokr
left a comment
crazyrokr
left a comment
There was a problem hiding this comment.
Please cover new code with tests
| protected void retryDeliveringImpl(ExternalNetworkMqttUser user, MqttSession session, Publish publish) { | ||
| int messageId = publish.messageId(); | ||
| MessageTacker messageTacker = session.outMessageTracker(); | ||
| TrackedMessageMeta messageMeta = messageTacker.stored(messageId); |
There was a problem hiding this comment.
Could we consider to rename messageTacker.stored to messageTacker.getStoredMessageMeta ?
There was a problem hiding this comment.
@crazyrokr message tracker contains only message meta, why do I need to repeat it in the method name?
There was a problem hiding this comment.
Neither the class name nor the variable name of the message tracker contains any reference to message metadataю Moreover, in the current implementation, calling the stored method looks like calling a setter, which does not imply returning a value from the method.
...n/java/javasabr/mqtt/service/publish/handler/impl/TrackableMqttPublishOutMessageHandler.java
Show resolved
Hide resolved
...c/main/java/javasabr/mqtt/service/publish/handler/impl/Qos2MqttPublishOutMessageHandler.java
Outdated
Show resolved
Hide resolved
...c/main/java/javasabr/mqtt/service/publish/handler/impl/Qos1MqttPublishOutMessageHandler.java
Show resolved
Hide resolved
...c/main/java/javasabr/mqtt/service/publish/handler/impl/Qos1MqttPublishOutMessageHandler.java
Show resolved
Hide resolved
|
|
||
| PublishReleaseReasonCode releaseResult; | ||
| // we unknown this flow | ||
| if (trackedMessageMeta == null) { |
There was a problem hiding this comment.
It might make sense to use } else if { branch here
There was a problem hiding this comment.
Something like this
PublishReleaseReasonCode releaseResult;
if (reasonCode != PublishReceivedReasonCode.SUCCESS) {
log.warning(clientId, reasonCode, messageId,
"[%s] Received error response:[%s] for publish:[%s]"::formatted);
// we can cancel the flow
if (trackedMessageMeta != null) {
messageTacker.remove(messageId);
}
return true;
} else if (trackedMessageMeta == null) { // we unknown this flow
releaseResult = PublishReleaseReasonCode.PACKET_IDENTIFIER_NOT_FOUND;
} else {
releaseResult = PublishReleaseReasonCode.SUCCESS;
messageTacker.update(messageId, MqttMessageType.PUBLISH_RELEASE, reasonCode);
}
...n/java/javasabr/mqtt/service/publish/handler/impl/TrackableMqttPublishOutMessageHandler.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // finish the flow | ||
| MessageTacker messageTacker = session.outMessageTracker(); |
There was a problem hiding this comment.
Does it make sense to use default branch here?
There was a problem hiding this comment.
So it would be something like this
PublishCompletedReasonCode reasonCode = publishComplete.reasonCode();
if (trackedMessageMeta == null) {
log.warning(clientId, messageId, "[%s] No any stored information for messageId:[%d]"::formatted);
return;
} else if (trackedMessageMeta.messageType() != MqttMessageType.PUBLISH_RELEASE) {
log.warning(clientId, trackedMessageMeta, messageId,
"[%s] No expected message meta:[%s] for messageId:[%d]"::formatted);
return;
} else if (reasonCode != PublishCompletedReasonCode.SUCCESS) {
log.warning(clientId, reasonCode, messageId,
"[%s] Received error response:[%s] for publish:[%s]"::formatted);
} else {
// finish the flow
MessageTacker messageTacker = session.outMessageTracker();
messageTacker.remove(messageId);
}
| } | ||
|
|
||
| PublishCompletedReasonCode reasonCode = publishComplete.reasonCode(); | ||
| if (reasonCode != PublishCompletedReasonCode.SUCCESS) { |
There was a problem hiding this comment.
It might make sense to use } else if { branch here
Improve publish delivering to subscribers, part 1