You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/10 02:02:10 UTC
[GitHub] [pulsar] aloyszhang opened a new pull request #11613: avoid non-durable subscription misuse an existed durable subscription
aloyszhang opened a new pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613
Fixes #11612
### Motivation
As described in #11612 , if a reader (or non durable subscription) use the same subscription name with an existed durable subscription, the reader will not behaves as expected.
This pull request add an check for this situation to prevent non-durable subscription misuse an existed durable subscription
### Modifications
When get the non-durable subscription and if the subscription existed check whether it's durable or not first.
And if there is a durable subscription already, return NotAllowedException.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
`NonDurableSubscriptionTest.testSameSubscriptionNameForDurableAndNonDurableSubscription`
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
No doce need
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685724888
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/NonDurableSubscriptionTest.java
##########
@@ -94,6 +94,53 @@ public void testNonDurableSubscription() throws Exception {
}
+ @Test
+ public void testSameSubscriptionNameForDurableAndNonDurableSubscription() throws Exception {
+ String topicName = "persistent://my-property/my-ns/same-sub-name-topic";
+ // 1 setup producer and consumer
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING).topic(topicName)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING).topic(topicName)
+ .readCompacted(true)
+ .subscriptionMode(SubscriptionMode.Durable)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .subscriptionName("mix-subscription")
+ .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest)
+ .subscribe();
+
+ // 2 send 10 messages
Review comment:
Oh, yes, you're right.
This test is firstly used for showing the behaviour before this pull request, at that point the reader start from the eariliest only can read 5 messages.
After this pull request, it seems no need to keep this logic.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-895880668
> Do we have the same problem but when you create a NonDurable subscription and then you use the same name to create a Durable subscription =
Yes, this also has the same problem and I have also add check logic.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-895777646
@eolivelli apply your comments , please take a look again.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-895880668
> Do we have the same problem but when you create a NonDurable subscription and then you use the same name to create a Durable subscription =
Yes, this also has the same problem and I have also add check logic.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685724888
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/NonDurableSubscriptionTest.java
##########
@@ -94,6 +94,53 @@ public void testNonDurableSubscription() throws Exception {
}
+ @Test
+ public void testSameSubscriptionNameForDurableAndNonDurableSubscription() throws Exception {
+ String topicName = "persistent://my-property/my-ns/same-sub-name-topic";
+ // 1 setup producer and consumer
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING).topic(topicName)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING).topic(topicName)
+ .readCompacted(true)
+ .subscriptionMode(SubscriptionMode.Durable)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .subscriptionName("mix-subscription")
+ .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest)
+ .subscribe();
+
+ // 2 send 10 messages
Review comment:
Oh, yes, you're right.
This test is firstly used for showing the behaviour before this pull request, at that point the reader start from the eariliest only can read 5 messages.
After this pull request, it seems no need to keep this logic.
Resolved @eolivelli PATL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685688888
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -898,6 +898,12 @@ public void openCursorFailed(ManagedLedgerException exception, Object ctx) {
subscription = new PersistentSubscription(this, subscriptionName, cursor, false);
subscriptions.put(subscriptionName, subscription);
+ } else {
+ // if subscription exist, check if it's a durable subscription
+ if (subscription.getCursor() != null && subscription.getCursor().isDurable()) {
+ return FutureUtil.failedFuture(
+ new NotAllowedException("Durable subscribe with the same name already exist."));
Review comment:
```suggestion
new NotAllowedException("Durable subscription with the same name already exists."));
```
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-895925836
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-895880668
> Do we have the same problem but when you create a NonDurable subscription and then you use the same name to create a Durable subscription =
Yes, this also has the same problem and I have add check logic too.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685724888
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/NonDurableSubscriptionTest.java
##########
@@ -94,6 +94,53 @@ public void testNonDurableSubscription() throws Exception {
}
+ @Test
+ public void testSameSubscriptionNameForDurableAndNonDurableSubscription() throws Exception {
+ String topicName = "persistent://my-property/my-ns/same-sub-name-topic";
+ // 1 setup producer and consumer
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING).topic(topicName)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING).topic(topicName)
+ .readCompacted(true)
+ .subscriptionMode(SubscriptionMode.Durable)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .subscriptionName("mix-subscription")
+ .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest)
+ .subscribe();
+
+ // 2 send 10 messages
Review comment:
Oh, yes, you're right.
This test is firstly used for showing the behaviour before this pull request, at that point the reader start from the eariliest can only read 5 messages.
After this pull request, it seems no need to keep this logic.
Resolved
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-895722412
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang removed a comment on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang removed a comment on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-896123481
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685690801
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -898,6 +898,12 @@ public void openCursorFailed(ManagedLedgerException exception, Object ctx) {
subscription = new PersistentSubscription(this, subscriptionName, cursor, false);
subscriptions.put(subscriptionName, subscription);
+ } else {
+ // if subscription exist, check if it's a durable subscription
Review comment:
```suggestion
// if subscription exists, check if it's a durable subscription
```
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-896123481
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685707535
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/NonDurableSubscriptionTest.java
##########
@@ -94,6 +94,53 @@ public void testNonDurableSubscription() throws Exception {
}
+ @Test
+ public void testSameSubscriptionNameForDurableAndNonDurableSubscription() throws Exception {
+ String topicName = "persistent://my-property/my-ns/same-sub-name-topic";
+ // 1 setup producer and consumer
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING).topic(topicName)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING).topic(topicName)
+ .readCompacted(true)
+ .subscriptionMode(SubscriptionMode.Durable)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .subscriptionName("mix-subscription")
+ .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest)
+ .subscribe();
+
+ // 2 send 10 messages
Review comment:
why do we need to send all these messages ?
isn't it enough to only create a Consumer and a Reader with the same subscription name ?
we can also make it simpler and more explicit to create:
- 1 Consumer with SubscriptionMode.Durable
- 1 Consumer with SubscriptionMode.NonDurable
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#issuecomment-896151514
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #11613: avoid non-durable subscription misuse an existed durable subscription
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #11613:
URL: https://github.com/apache/pulsar/pull/11613#discussion_r685724888
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/NonDurableSubscriptionTest.java
##########
@@ -94,6 +94,53 @@ public void testNonDurableSubscription() throws Exception {
}
+ @Test
+ public void testSameSubscriptionNameForDurableAndNonDurableSubscription() throws Exception {
+ String topicName = "persistent://my-property/my-ns/same-sub-name-topic";
+ // 1 setup producer and consumer
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING).topic(topicName)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING).topic(topicName)
+ .readCompacted(true)
+ .subscriptionMode(SubscriptionMode.Durable)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .subscriptionName("mix-subscription")
+ .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest)
+ .subscribe();
+
+ // 2 send 10 messages
Review comment:
Oh, yes, you're right.
This test is firstly used for showing the behaviour before this pull request, at that point the reader start from the eariliest can only read 5 messages.
After this pull request, it seems no need to keep this logic.
Resolved @eolivelli PATL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org