Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

Also persist QoS 0 subscriptions when clean flag false #694

Merged
merged 3 commits into from Oct 30, 2017

Conversation

mitsos1os
Copy link
Contributor

No description provided.

@mitsos1os
Copy link
Contributor Author

@mcollina Please check what is going on with the Travis Build and it fails.. .Locally with Node v8.7.0 everything succeeds.. Thanks

@mitsos1os
Copy link
Contributor Author

@mcollina Sorry to insist on the issue, but because we are using Mosca in a production environment, and this bug affects us, I would prefer to use the corrected version instead of monkey patching the module code after running, could you please check if anything else is needed for the Travis Build to work? Because I think the errors are irrelevant, since in my local environment all tests succeed.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

This is good to go. CI is fine, and I'll land if there is one of those intermittent failures that I cannot track.

//if (client.subscriptions[key].qos > 0) { // Issue #693
subscriptions[key] = client.subscriptions[key];
subscriptions[key].ttl = that.options.ttl.subscriptions + now;
//} // Issue #693
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove these comments?

}
// if (client.subscriptions[key].qos > 0) { // Issue #693
subscriptions[key] = client.subscriptions[key];
//} // Issue #693
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove these comments?

@@ -558,7 +558,7 @@ module.exports = function(create, buildOpts) {
});
});

it("should not store a QoS 0 subscription", function(done) {
it("should store a QoS 0 subscription", function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please check/add a test that we are not enqueuing QoS 0 packets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you tell me what you mean? You already have test for QoS 0 packages

Copy link
Collaborator

Choose a reason for hiding this comment

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

forget my comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eeeee... what comment???? ;)

@mitsos1os
Copy link
Contributor Author

Ok just removed the comments! Thanks!

@mcollina mcollina merged commit 5277cca into moscajs:master Oct 30, 2017
@mcollina
Copy link
Collaborator

Thanks! Releasing asap.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants