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

Failure to attain QoS is not bad #1845

Merged
merged 1 commit into from Aug 1, 2019
Merged

Failure to attain QoS is not bad #1845

merged 1 commit into from Aug 1, 2019

Conversation

huntc
Copy link
Contributor

@huntc huntc commented Jul 28, 2019

Prior to this PR, a failure to attain a QoS level when subscribing to a topic was considered bad. The spec treats this condition as one being something for the application to handle e.g. a client can decide to proceed or not given that one of the topics being subscribed fails or not.

The fix is to deprecate the BadSubAckMessage and always return SubAck. This then permits the Subscriber to process SubAcks whether they are good or bad from the application's perspective.

Fixes ##1833 (comment)

Prior to this commit, a failure to attain a QoS level i.e. subscribe, was considered bad. The spec treats this condition as one being something for the application to handle e.g. a client can decide to proceed or not given that one of the topics being subscribed was able to be so.

The fix is to deprecate the BadSubAckMessage and always return SubAck. This then permits the Subscriber to process SubAcks whether they are good or bad from the application's perspective.
@huntc
Copy link
Contributor Author

huntc commented Jul 28, 2019

CI is failing - although I suspect the test is flaky given the multitude number of times CI has been passing. Any advice?

@idarlington
Copy link

idarlington commented Jul 29, 2019

@huntc

I built this locally and tested it. I can confirm that a SubAck is received for partially successful subscriptions and SubscribeFailed is no longer thrown.

Decoded SubAck:

[info] 2019-07-29 10:42:45,642 DEBUG a.s.Materializer [default-akka.actor.default-dispatcher-7] [client-events] Element: Right(SubAck(PacketId(1),Vector(ControlPacketFlags(0), ControlPacketFlags(0), ControlPacketFlags(128), ControlPacketFlags(128), ControlPacketFlags(128), ControlPacketFlags(128), ControlPacketFlags(128), ControlPacketFlags(0), ControlPacketFlags(0))))

@huntc
Copy link
Contributor Author

huntc commented Jul 29, 2019

Great. Thanks for confirming.

This PR is ready for review.

@huntc
Copy link
Contributor Author

huntc commented Jul 31, 2019

ping @2m

Copy link
Member

@2m 2m left a comment

Choose a reason for hiding this comment

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

LGTM!

@2m 2m added this to the 1.1.1 milestone Aug 1, 2019
@2m 2m merged commit 7eec2ea into akka:master Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants