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 2022/09/13 07:34:31 UTC
[GitHub] [pulsar] eolivelli opened a new pull request, #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
eolivelli opened a new pull request, #17609:
URL: https://github.com/apache/pulsar/pull/17609
### Motivation
During the deletion of a namespace we don't want that autoTopicCreation triggers the creation of topics
### Modifications
Consider "Policies.deleted" while deciding if autoTopicCreation is available or not
### Verifying this change
I will add tests
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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 pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#issuecomment-1245442227
> When the namespace deletion instruction is not processed by the current broker, may succeed in creating the topic because the meta data has a brief cache.
This is a good point, but I would defer it to the most bigger issue of cache consistency
--
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 pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#issuecomment-1245205832
@poorbarcode unfortunately my test case doesn't cover well the change, because basically the automatic topic creation is prevented earlier, because we have the check about policies.deleted in PulsarWebResources.checkLocalOrGetPeerReplicationCluster()
do you have some suggestions ?
--
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 diff in pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#discussion_r970381253
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicAutoCreationTest.java:
##########
@@ -85,4 +105,76 @@ public void testPartitionedTopicAutoCreation() throws PulsarAdminException, Puls
producer.close();
}
+
+
+ @Test
+ public void testPartitionedTopicAutoCreationForbiddenDuringNamespaceDeletion()
+ throws Exception {
+ final String namespaceName = "my-property/my-ns";
+ final String topic = "persistent://" + namespaceName + "/test-partitioned-topi-auto-creation-"
+ + UUID.randomUUID().toString();
+
+ pulsar.getPulsarResources().getNamespaceResources()
+ .setPolicies(NamespaceName.get(namespaceName), old -> {
+ old.deleted = true;
+ return old;
+ });
+
+
+ LookupService original = Whitebox.getInternalState(pulsarClient, "lookup");
+ try {
+
+ // we want to skip the "lookup" phase, because it is blocked by the HTTP API
+ LookupService mockLookup = mock(LookupService.class);
+ Whitebox.setInternalState(pulsarClient, "lookup", mockLookup);
+ when(mockLookup.getPartitionedTopicMetadata(any())).thenAnswer(i -> {
+ return CompletableFuture.completedFuture(new PartitionedTopicMetadata(0));
+ });
+ when(mockLookup.getBroker(any())).thenAnswer(i -> {
+ InetSocketAddress brokerAddress =
+ new InetSocketAddress(pulsar.getAdvertisedAddress(), pulsar.getBrokerListenPort().get());
+ return CompletableFuture.completedFuture(Pair.of(brokerAddress, brokerAddress));
+ });
+
+ // Creating a producer and creating a Consumer may trigger automatic topic
+ // creation, let's try to create a Producer and a Consumer
+ try (Producer<byte[]> producer = pulsarClient.newProducer()
+ .sendTimeout(1, TimeUnit.SECONDS)
+ .topic(topic)
+ .create();) {
+ } catch (PulsarClientException.LookupException expected) {
Review Comment:
This is happening because the broker hasn't loaded the bundle.
because basically the producer/consumer could not set them up.
I wasn't sure to add an assertion about "Namespace bundle for topic (%s) not served by this instance"
btw maybe it is better, so that the behaviour changes we will see this test failing
--
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 #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #17609:
URL: https://github.com/apache/pulsar/pull/17609
--
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 pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#issuecomment-1245202374
@poorbarcode @dlg99 PTAL
--
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 diff in pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#discussion_r970466673
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicAutoCreationTest.java:
##########
@@ -85,4 +105,76 @@ public void testPartitionedTopicAutoCreation() throws PulsarAdminException, Puls
producer.close();
}
+
+
+ @Test
+ public void testPartitionedTopicAutoCreationForbiddenDuringNamespaceDeletion()
+ throws Exception {
+ final String namespaceName = "my-property/my-ns";
+ final String topic = "persistent://" + namespaceName + "/test-partitioned-topi-auto-creation-"
+ + UUID.randomUUID().toString();
+
+ pulsar.getPulsarResources().getNamespaceResources()
+ .setPolicies(NamespaceName.get(namespaceName), old -> {
+ old.deleted = true;
+ return old;
+ });
+
+
+ LookupService original = Whitebox.getInternalState(pulsarClient, "lookup");
+ try {
+
+ // we want to skip the "lookup" phase, because it is blocked by the HTTP API
+ LookupService mockLookup = mock(LookupService.class);
+ Whitebox.setInternalState(pulsarClient, "lookup", mockLookup);
+ when(mockLookup.getPartitionedTopicMetadata(any())).thenAnswer(i -> {
+ return CompletableFuture.completedFuture(new PartitionedTopicMetadata(0));
+ });
+ when(mockLookup.getBroker(any())).thenAnswer(i -> {
+ InetSocketAddress brokerAddress =
+ new InetSocketAddress(pulsar.getAdvertisedAddress(), pulsar.getBrokerListenPort().get());
+ return CompletableFuture.completedFuture(Pair.of(brokerAddress, brokerAddress));
+ });
+
+ // Creating a producer and creating a Consumer may trigger automatic topic
+ // creation, let's try to create a Producer and a Consumer
+ try (Producer<byte[]> producer = pulsarClient.newProducer()
+ .sendTimeout(1, TimeUnit.SECONDS)
+ .topic(topic)
+ .create();) {
+ } catch (PulsarClientException.LookupException expected) {
Review Comment:
done
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicAutoCreationTest.java:
##########
@@ -85,4 +105,76 @@ public void testPartitionedTopicAutoCreation() throws PulsarAdminException, Puls
producer.close();
}
+
+
+ @Test
+ public void testPartitionedTopicAutoCreationForbiddenDuringNamespaceDeletion()
+ throws Exception {
+ final String namespaceName = "my-property/my-ns";
+ final String topic = "persistent://" + namespaceName + "/test-partitioned-topi-auto-creation-"
+ + UUID.randomUUID().toString();
+
+ pulsar.getPulsarResources().getNamespaceResources()
+ .setPolicies(NamespaceName.get(namespaceName), old -> {
+ old.deleted = true;
+ return old;
+ });
+
+
+ LookupService original = Whitebox.getInternalState(pulsarClient, "lookup");
+ try {
+
+ // we want to skip the "lookup" phase, because it is blocked by the HTTP API
+ LookupService mockLookup = mock(LookupService.class);
+ Whitebox.setInternalState(pulsarClient, "lookup", mockLookup);
+ when(mockLookup.getPartitionedTopicMetadata(any())).thenAnswer(i -> {
+ return CompletableFuture.completedFuture(new PartitionedTopicMetadata(0));
+ });
+ when(mockLookup.getBroker(any())).thenAnswer(i -> {
+ InetSocketAddress brokerAddress =
+ new InetSocketAddress(pulsar.getAdvertisedAddress(), pulsar.getBrokerListenPort().get());
+ return CompletableFuture.completedFuture(Pair.of(brokerAddress, brokerAddress));
+ });
+
+ // Creating a producer and creating a Consumer may trigger automatic topic
+ // creation, let's try to create a Producer and a Consumer
+ try (Producer<byte[]> producer = pulsarClient.newProducer()
+ .sendTimeout(1, TimeUnit.SECONDS)
+ .topic(topic)
+ .create();) {
+ } catch (PulsarClientException.LookupException expected) {
+ }
+
+ try (Consumer<byte[]> consumer = pulsarClient.newConsumer()
+ .topic(topic)
+ .subscriptionName("test")
+ .subscribe();) {
+ } catch (PulsarClientException.LookupException expected) {
Review Comment:
done
--
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 pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#issuecomment-1245443644
@poorbarcode I added another test that demonstrates that this patch is covering one edge case that you mentioned.
We must prevent auto topic creation not only during lookup but also while creating a consumer or a producer.
The new test covers this change now
--
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] poorbarcode commented on a diff in pull request #17609: [bugfix] Prevent Automatic Topic Creation during namespace deletion
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #17609:
URL: https://github.com/apache/pulsar/pull/17609#discussion_r969861170
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicAutoCreationTest.java:
##########
@@ -85,4 +105,76 @@ public void testPartitionedTopicAutoCreation() throws PulsarAdminException, Puls
producer.close();
}
+
+
+ @Test
+ public void testPartitionedTopicAutoCreationForbiddenDuringNamespaceDeletion()
+ throws Exception {
+ final String namespaceName = "my-property/my-ns";
+ final String topic = "persistent://" + namespaceName + "/test-partitioned-topi-auto-creation-"
+ + UUID.randomUUID().toString();
+
+ pulsar.getPulsarResources().getNamespaceResources()
+ .setPolicies(NamespaceName.get(namespaceName), old -> {
+ old.deleted = true;
+ return old;
+ });
+
+
+ LookupService original = Whitebox.getInternalState(pulsarClient, "lookup");
+ try {
+
+ // we want to skip the "lookup" phase, because it is blocked by the HTTP API
+ LookupService mockLookup = mock(LookupService.class);
+ Whitebox.setInternalState(pulsarClient, "lookup", mockLookup);
+ when(mockLookup.getPartitionedTopicMetadata(any())).thenAnswer(i -> {
+ return CompletableFuture.completedFuture(new PartitionedTopicMetadata(0));
+ });
+ when(mockLookup.getBroker(any())).thenAnswer(i -> {
+ InetSocketAddress brokerAddress =
+ new InetSocketAddress(pulsar.getAdvertisedAddress(), pulsar.getBrokerListenPort().get());
+ return CompletableFuture.completedFuture(Pair.of(brokerAddress, brokerAddress));
+ });
+
+ // Creating a producer and creating a Consumer may trigger automatic topic
+ // creation, let's try to create a Producer and a Consumer
+ try (Producer<byte[]> producer = pulsarClient.newProducer()
+ .sendTimeout(1, TimeUnit.SECONDS)
+ .topic(topic)
+ .create();) {
+ } catch (PulsarClientException.LookupException expected) {
+ }
+
+ try (Consumer<byte[]> consumer = pulsarClient.newConsumer()
+ .topic(topic)
+ .subscriptionName("test")
+ .subscribe();) {
+ } catch (PulsarClientException.LookupException expected) {
Review Comment:
Same as above
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicAutoCreationTest.java:
##########
@@ -85,4 +105,76 @@ public void testPartitionedTopicAutoCreation() throws PulsarAdminException, Puls
producer.close();
}
+
+
+ @Test
+ public void testPartitionedTopicAutoCreationForbiddenDuringNamespaceDeletion()
+ throws Exception {
+ final String namespaceName = "my-property/my-ns";
+ final String topic = "persistent://" + namespaceName + "/test-partitioned-topi-auto-creation-"
+ + UUID.randomUUID().toString();
+
+ pulsar.getPulsarResources().getNamespaceResources()
+ .setPolicies(NamespaceName.get(namespaceName), old -> {
+ old.deleted = true;
+ return old;
+ });
+
+
+ LookupService original = Whitebox.getInternalState(pulsarClient, "lookup");
+ try {
+
+ // we want to skip the "lookup" phase, because it is blocked by the HTTP API
+ LookupService mockLookup = mock(LookupService.class);
+ Whitebox.setInternalState(pulsarClient, "lookup", mockLookup);
+ when(mockLookup.getPartitionedTopicMetadata(any())).thenAnswer(i -> {
+ return CompletableFuture.completedFuture(new PartitionedTopicMetadata(0));
+ });
+ when(mockLookup.getBroker(any())).thenAnswer(i -> {
+ InetSocketAddress brokerAddress =
+ new InetSocketAddress(pulsar.getAdvertisedAddress(), pulsar.getBrokerListenPort().get());
+ return CompletableFuture.completedFuture(Pair.of(brokerAddress, brokerAddress));
+ });
+
+ // Creating a producer and creating a Consumer may trigger automatic topic
+ // creation, let's try to create a Producer and a Consumer
+ try (Producer<byte[]> producer = pulsarClient.newProducer()
+ .sendTimeout(1, TimeUnit.SECONDS)
+ .topic(topic)
+ .create();) {
+ } catch (PulsarClientException.LookupException expected) {
Review Comment:
Should we add validation to make sure the exception is what we expect? When I run this test, the exception is "Namespace bundle for topic (%s) not served by this instance" which should not be what we expected.
--
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