You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by jbertram <gi...@git.apache.org> on 2019/01/08 02:44:05 UTC
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
GitHub user jbertram opened a pull request:
https://github.com/apache/activemq-artemis/pull/2491
ARTEMIS-2217 remove state on clean MQTT session disconnect
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-2217
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2491.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2491
----
commit a4d0cf9ab42dd5076b78fb47e3425d11c917db53
Author: Justin Bertram <jb...@...>
Date: 2018-12-29T08:53:04Z
ARTEMIS-2217 remove state on clean MQTT session disconnect
----
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by onlyMIT <gi...@git.apache.org>.
Github user onlyMIT commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2491#discussion_r245878622
--- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSession.java ---
@@ -117,14 +118,11 @@ boolean getStopped() {
}
boolean isClean() {
- return isClean;
+ return clean;
}
- void setIsClean(boolean isClean) throws Exception {
- this.isClean = isClean;
- if (isClean) {
- clean();
- }
+ void setClean(boolean clean) throws Exception {
+ this.clean = clean;
--- End diff --
It is necessary to call the "clean()" method to clean up old session information when creating a connection.
If it is not cleaned up, when the cleanSession of the last MQTT consumer is false, and the cleanSession of the connected MQTT consumer is true, the message in the old queue will be consumed, which is actually not allowed.
I think this is why calling "clean()" in the "setIsClean(boolean isClean)" method
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by onlyMIT <gi...@git.apache.org>.
Github user onlyMIT commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2491#discussion_r246247094
--- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSession.java ---
@@ -117,14 +118,11 @@ boolean getStopped() {
}
boolean isClean() {
- return isClean;
+ return clean;
}
- void setIsClean(boolean isClean) throws Exception {
- this.isClean = isClean;
- if (isClean) {
- clean();
- }
+ void setClean(boolean clean) throws Exception {
+ this.clean = clean;
--- End diff --
Look good to me!
---
[GitHub] activemq-artemis issue #2491: ARTEMIS-2217 remove state on clean MQTT sessio...
Posted by onlyMIT <gi...@git.apache.org>.
Github user onlyMIT commented on the issue:
https://github.com/apache/activemq-artemis/pull/2491
@jbertram I think I found out why your solution passed the test， your test was constructed on a wrong test code. the solution your provided, after I tested it with the revised test code, the test did not pass.
The test issue， i created a jira and opened a [#2493](https://github.com/apache/activemq-artemis/pull/2493) to solve this test issue.
we need it， calling "clean()" in the "setIsClean(boolean isClean)" method.
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2491
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by onlyMIT <gi...@git.apache.org>.
Github user onlyMIT commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2491#discussion_r245879352
--- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSession.java ---
@@ -117,14 +118,11 @@ boolean getStopped() {
}
boolean isClean() {
- return isClean;
+ return clean;
}
- void setIsClean(boolean isClean) throws Exception {
- this.isClean = isClean;
- if (isClean) {
- clean();
- }
+ void setClean(boolean clean) throws Exception {
+ this.clean = clean;
--- End diff --
It is necessary to call the "clean()" method to clean up old session information when creating a connection.
If it is not cleaned up, when the cleanSession of the last MQTT consumer is false, and the cleanSession of the connected MQTT consumer is true, the message in the old queue will be consumed, which is actually not allowed.
I think this is why calling "clean()" in the "setIsClean(boolean isClean)" method
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2491#discussion_r246075935
--- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSession.java ---
@@ -117,14 +118,11 @@ boolean getStopped() {
}
boolean isClean() {
- return isClean;
+ return clean;
}
- void setIsClean(boolean isClean) throws Exception {
- this.isClean = isClean;
- if (isClean) {
- clean();
- }
+ void setClean(boolean clean) throws Exception {
+ this.clean = clean;
--- End diff --
The expectation of a "setter" method is simply to _set_ a variable and nothing more. The additional logic in the `setClean` method is not intuitive which is why I removed it and put it in the `getSessionState` method.
---
[GitHub] activemq-artemis issue #2491: ARTEMIS-2217 remove state on clean MQTT sessio...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2491
IMO this should replace #2485.
---
[GitHub] activemq-artemis issue #2491: ARTEMIS-2217 remove state on clean MQTT sessio...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2491
The test code is correct. The state kept in an unclean session includes the subscriptions which is why the test doesn't create a subscription before trying to receive the message. If the broker doesn't properly clean the session state when the client connects with cleanSession=true then it will receive a message and the test will fail.
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by onlyMIT <gi...@git.apache.org>.
Github user onlyMIT commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2491#discussion_r246095670
--- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSession.java ---
@@ -117,14 +118,11 @@ boolean getStopped() {
}
boolean isClean() {
- return isClean;
+ return clean;
}
- void setIsClean(boolean isClean) throws Exception {
- this.isClean = isClean;
- if (isClean) {
- clean();
- }
+ void setClean(boolean clean) throws Exception {
+ this.clean = clean;
--- End diff --
@jbertram in the getSessionState method.Only clear state，not call 'clean()' method. In fact, the queue is not cleaned up.
I use the code for the ‘paho’ test. The first consumer "cleanSession=false", using a different clientID to open a producer to send a message. Close the producer and consumer, use the same clientID and cleanSession = true" to open the second consumer and find that the consumer will consume the legacy message in the queue。So I suspect that there is a problem with the test code.
I am always looking for why my test results will consume the legacy messages in the queue, and your test results will not。
After seeing your information, I re-reviewed the code and found that the test code did not have any problems. What is causing my doubts is that because of your change, when cleanSession=true, only the MQTTSessionState is cleaned up, the queue still exists, and the legacy messages in the queue are consumed when resubscribing.
Can close [#2493 ](https://github.com/apache/activemq-artemis/pull/2493) . I think you need to review your change。
---
[GitHub] activemq-artemis pull request #2491: ARTEMIS-2217 remove state on clean MQTT...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2491#discussion_r246123082
--- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTSession.java ---
@@ -117,14 +118,11 @@ boolean getStopped() {
}
boolean isClean() {
- return isClean;
+ return clean;
}
- void setIsClean(boolean isClean) throws Exception {
- this.isClean = isClean;
- if (isClean) {
- clean();
- }
+ void setClean(boolean clean) throws Exception {
+ this.clean = clean;
--- End diff --
I pushed an update to address the issue you identified. Thanks!
---