You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/03/11 16:35:48 UTC
[GitHub] [rocketmq-mqtt] ferrirW opened a new pull request #25: [ISSUE #19] fix '#' check in topicFilter
ferrirW opened a new pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25
link #19
some code can be optimized:
1. ADDFLAG & JINFLAG should be replaced with PLUS_SIGN & NUMBER_SIGN refer to unicode
2. Some redundant containsKey before putIfAbsent()
3. buildIntanceName -> buildInstanceName
4. TopicUtils.isMatch("t/t1/t2/", "t/#/t2/") should be false but return true now.(I know some client will do this check like poho, however, it is better for the server to do a validation as well)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#issuecomment-1066288119
please make sure the ci pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] ShannonDing merged pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
ShannonDing merged pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] ShannonDing commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
ShannonDing commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r825545817
##########
File path: mqtt-common/src/test/java/org/apache/rocketmq/mqtt/common/test/TestTopicUtils.java
##########
@@ -0,0 +1,43 @@
+package org.apache.rocketmq.mqtt.common.test;
Review comment:
Lack of license header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r825562558
##########
File path: mqtt-common/src/test/java/org/apache/rocketmq/mqtt/common/test/TestTopicUtils.java
##########
@@ -0,0 +1,43 @@
+package org.apache.rocketmq.mqtt.common.test;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.rocketmq.mqtt.common.model.Trie;
+import org.apache.rocketmq.mqtt.common.util.TopicUtils;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestTopicUtils {
+
+ @Test
+ public void testTopicMatch() {
+ String topic = "t/t1/t2/";
+ String topicFilter = "t/t1/t2/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/+/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/+/#/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/t2/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/+/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/t2/t3/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/#/t3/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1#/";
Review comment:
Will "t/t1#/" appear?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r825564078
##########
File path: mqtt-ds/src/main/java/org/apache/rocketmq/mqtt/ds/meta/WildcardManager.java
##########
@@ -99,9 +99,7 @@ private void refreshWildcards(String firstTopic) {
queueNames.add(pubTopic);
Set<String> wildcards = matchWildcards(pubTopic);
if (wildcards != null && !wildcards.isEmpty()) {
Review comment:
line 101 the return wildcards may never be null and the if judgment can be removed or not ？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] ferrirW commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r825640255
##########
File path: mqtt-ds/src/main/java/org/apache/rocketmq/mqtt/ds/meta/WildcardManager.java
##########
@@ -99,9 +99,7 @@ private void refreshWildcards(String firstTopic) {
queueNames.add(pubTopic);
Set<String> wildcards = matchWildcards(pubTopic);
if (wildcards != null && !wildcards.isEmpty()) {
Review comment:
I prefer not to removed. It's not expensive and could avoid problems that may arise from a change of matchWildcards() in the future.
##########
File path: mqtt-common/src/test/java/org/apache/rocketmq/mqtt/common/test/TestTopicUtils.java
##########
@@ -0,0 +1,43 @@
+package org.apache.rocketmq.mqtt.common.test;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.rocketmq.mqtt.common.model.Trie;
+import org.apache.rocketmq.mqtt.common.util.TopicUtils;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestTopicUtils {
+
+ @Test
+ public void testTopicMatch() {
+ String topic = "t/t1/t2/";
+ String topicFilter = "t/t1/t2/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/+/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/+/#/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/t2/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/+/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/t2/t3/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/#/t3/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1#/";
Review comment:
> Will "t/t1#/" appear?
Maybe misuse. Clients probably reject this type, but it's better for the server to check it itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r825762585
##########
File path: mqtt-ds/src/main/java/org/apache/rocketmq/mqtt/ds/meta/WildcardManager.java
##########
@@ -99,9 +99,7 @@ private void refreshWildcards(String firstTopic) {
queueNames.add(pubTopic);
Set<String> wildcards = matchWildcards(pubTopic);
if (wildcards != null && !wildcards.isEmpty()) {
Review comment:
agree that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] pingww commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
pingww commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r826525702
##########
File path: mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/InFlyCache.java
##########
@@ -61,9 +61,7 @@ public void cleanResource(String clientId,String channelId) {
public void put(CacheType cacheType, String channelId, int mqttMsgId) {
ConcurrentMap<String, Set<Integer>> cache = whichCache(cacheType);
- if (!cache.containsKey(channelId)) {
Review comment:
Adding judgment is mainly to avoid unnecessary temporary object generation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r825761851
##########
File path: mqtt-common/src/test/java/org/apache/rocketmq/mqtt/common/test/TestTopicUtils.java
##########
@@ -0,0 +1,43 @@
+package org.apache.rocketmq.mqtt.common.test;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.rocketmq.mqtt.common.model.Trie;
+import org.apache.rocketmq.mqtt.common.util.TopicUtils;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestTopicUtils {
+
+ @Test
+ public void testTopicMatch() {
+ String topic = "t/t1/t2/";
+ String topicFilter = "t/t1/t2/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/+/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/+/#/";
+ Assert.assertTrue(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/t2/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/#/+/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/t2/t3/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1/#/t3/";
+ Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter));
+
+ topicFilter = "t/t1#/";
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] ferrirW commented on a change in pull request #25: [ISSUE #19] fix '#' check in topicFilter
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #25:
URL: https://github.com/apache/rocketmq-mqtt/pull/25#discussion_r826537466
##########
File path: mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/match/MatchAction.java
##########
@@ -120,9 +120,7 @@ public void addSubscription(Session session, Set<Subscription> subscriptions) {
}
synchronized (topicCache) {
- if (!topicCache.containsKey(topicFilter)) {
Review comment:
This place is for subscribe request, that will not be frequent, can I keep it? @pingww
##########
File path: mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/InFlyCache.java
##########
@@ -61,9 +61,7 @@ public void cleanResource(String clientId,String channelId) {
public void put(CacheType cacheType, String channelId, int mqttMsgId) {
ConcurrentMap<String, Set<Integer>> cache = whichCache(cacheType);
- if (!cache.containsKey(channelId)) {
Review comment:
Oh, that's my mistake. I just replaced them all, and don't notice this cache operation will be frequent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org