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/06/12 14:20:39 UTC
[GitHub] [pulsar] Radiancebobo opened a new pull request, #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Radiancebobo opened a new pull request, #16023:
URL: https://github.com/apache/pulsar/pull/16023
allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
Fixes #16020
### Motivation
In the broker.conf configuration, there are these configurations:
- allowAutoTopicCreation
- allowAutoTopicCreationType
- brokerDeleteInactiveTopicsEnabled
- brokerDeleteInactiveTopicsFrequencySeconds
- brokerDeleteInactiveTopicsMaxInactiveDurationSeconds
- allowAutoSubscriptionCreation
I think these should be dynamically configurable.
Pulsar has namespace-level policy, but the broker-level dynamically configuration is missing.
### Modifications
The change is very simple, just add dynamic identification to these configurations in ServiceConfiguration.java, and add a callback function when updating.
### Documentation
Need to update docs?
- [x] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164027845
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164049081
/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] BewareMyPower merged pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #16023:
URL: https://github.com/apache/pulsar/pull/16023
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164292468
/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] BewareMyPower commented on a diff in pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#discussion_r911586479
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/InactiveTopicDeleteTest.java:
##########
@@ -596,4 +597,64 @@ public void testHealthTopicInactiveNotClean() throws Exception {
Assert.assertTrue(V1Partitions.contains(healthCheckTopicV1));
Assert.assertTrue(V2Partitions.contains(healthCheckTopicV2));
}
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsEnabled() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsEnabled(true);
+ super.baseSetup();
+ admin.brokers().updateDynamicConfiguration("brokerDeleteInactiveTopicsEnabled", "false");
+ Awaitility.await().untilAsserted(()->{
+ assertFalse(conf.isBrokerDeleteInactiveTopicsEnabled());
+ });
+ }
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsFrequencySeconds() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsFrequencySeconds(30);
+ super.baseSetup();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactiveTopicsFrequencySeconds", "60");
+ Awaitility.await().untilAsserted(()->{
+ assertEquals(conf.getBrokerDeleteInactiveTopicsFrequencySeconds(), 60);
+ });
+ }
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsMaxInactiveDurationSeconds() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsMaxInactiveDurationSeconds(30);
+ super.baseSetup();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactiveTopicsMaxInactiveDurationSeconds", "60");
+ Awaitility.await().untilAsserted(()->{
+ assertEquals(conf.getBrokerDeleteInactiveTopicsMaxInactiveDurationSeconds(), 60);
+ });
+ }
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsMode() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsMode (InactiveTopicDeleteMode.delete_when_no_subscriptions);
+ super.baseSetup();
+ String expect = InactiveTopicDeleteMode.delete_when_subscriptions_caught_up.toString();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactiveTopicsMode",
+ expect);
+ Awaitility.await().untilAsserted(()->{
+ assertEquals(conf.getBrokerDeleteInactiveTopicsMode().toString(), expect);
+ });
+ }
+
+ @Test
+ public void testBrokerDeleteInactivePartitionedTopicMetadataEnabled() throws Exception {
+ conf.setBrokerDeleteInactivePartitionedTopicMetadataEnabled(false);
+ super.baseSetup();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactivePartitionedTopicMetadataEnabled",
+ "true");
+ Thread.sleep(2000);
+ Awaitility.await().untilAsserted(()->{
+ assertTrue(conf.isBrokerDeleteInactivePartitionedTopicMetadataEnabled());
+ });
+ }
Review Comment:
Don't use `Thread.sleep`, use `Awaitility.await().atMost(/* ... */)`.
I noticed some existing tests might also use `Thread.sleep`, but it's not good. For new tests, it's better not to sleep explicitly.
--
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] BewareMyPower commented on a diff in pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#discussion_r911590864
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceAutoSubscriptionCreationTest.java:
##########
@@ -152,4 +154,24 @@ public void testNonPersistentTopicSubscriptionCreationWithAutoCreationDisable()
assertTrue(admin.topics().getSubscriptions(topicName).contains(subscriptionName));
}
+ @Test
+ public void testDynamicConfigurationTopicAutoSubscriptionCreation()
+ throws PulsarAdminException, PulsarClientException {
+ pulsar.getConfiguration().setAllowAutoTopicCreation(false);
+ pulsar.getConfiguration().setAllowAutoSubscriptionCreation(true);
+ admin.brokers().updateDynamicConfiguration("allowAutoSubscriptionCreation", "false");
+ String topicString = "persistent://prop/ns-abc/non-partitioned-topic" + UUID.randomUUID();
+ String subscriptionName = "non-partitioned-topic-sub";
+ admin.topics().createNonPartitionedTopic(topicString);
+ try {
+ pulsarClient.newConsumer().topic(topicString).subscriptionName(subscriptionName).subscribe();
+ fail("Subscribe operation should have failed");
+ } catch (Exception e) {
+ assertTrue(e instanceof PulsarClientException);
+ }
Review Comment:
We can simplify this type of verification to
```java
Assert.assertThrows(PulsarClientException.class, () ->
pulsarClient.newConsumer().topic(topicString).subscriptionName(subscriptionName).subscribe());
```
I think we can also apply similar changes to `testDynamicConfigurationTopicAutoCreationDisable`.
--
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] github-actions[bot] commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1171976202
@Radiancebobo Please select only one documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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] Demogorgon314 commented on a diff in pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#discussion_r895622744
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceAutoTopicCreationTest.java:
##########
@@ -414,4 +419,105 @@ public void testAutoCreationOfSystemTopicNamespaceEvents() throws Exception {
assertTrue(admin.namespaces().getTopics("prop/ns-abc").contains(topicString));
assertFalse(admin.topics().getPartitionedTopicList("prop/ns-abc").contains(topicString));
}
+
+ @Test
+ public void testDynamicConfigurationTopicAutoCreationDisable() throws PulsarAdminException {
+ // test disable AllowAutoTopicCreation
+ pulsar.getConfiguration().setAllowAutoTopicCreation(true);
+ admin.brokers().updateDynamicConfiguration("allowAutoTopicCreation", "false");
+ final String namespaceName = "prop/ns-abc";
+ final String topic = "persistent://" + namespaceName + "/test-dynamicConfiguration-topic-auto-creation-"
+ + UUID.randomUUID();
+ try {
+ org.apache.pulsar.client.api.Producer<byte[]> producer = pulsarClient.newProducer()
Review Comment:
```suggestion
pulsarClient.newProducer()
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceAutoTopicCreationTest.java:
##########
@@ -414,4 +419,105 @@ public void testAutoCreationOfSystemTopicNamespaceEvents() throws Exception {
assertTrue(admin.namespaces().getTopics("prop/ns-abc").contains(topicString));
assertFalse(admin.topics().getPartitionedTopicList("prop/ns-abc").contains(topicString));
}
+
+ @Test
+ public void testDynamicConfigurationTopicAutoCreationDisable() throws PulsarAdminException {
+ // test disable AllowAutoTopicCreation
+ pulsar.getConfiguration().setAllowAutoTopicCreation(true);
+ admin.brokers().updateDynamicConfiguration("allowAutoTopicCreation", "false");
+ final String namespaceName = "prop/ns-abc";
+ final String topic = "persistent://" + namespaceName + "/test-dynamicConfiguration-topic-auto-creation-"
+ + UUID.randomUUID();
+ try {
+ org.apache.pulsar.client.api.Producer<byte[]> producer = pulsarClient.newProducer()
+ .topic(topic)
+ .create();
Review Comment:
```suggestion
.create();
fail();
```
Should add `fail()`.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceAutoTopicCreationTest.java:
##########
@@ -414,4 +419,105 @@ public void testAutoCreationOfSystemTopicNamespaceEvents() throws Exception {
assertTrue(admin.namespaces().getTopics("prop/ns-abc").contains(topicString));
assertFalse(admin.topics().getPartitionedTopicList("prop/ns-abc").contains(topicString));
}
+
+ @Test
+ public void testDynamicConfigurationTopicAutoCreationDisable() throws PulsarAdminException {
+ // test disable AllowAutoTopicCreation
+ pulsar.getConfiguration().setAllowAutoTopicCreation(true);
+ admin.brokers().updateDynamicConfiguration("allowAutoTopicCreation", "false");
+ final String namespaceName = "prop/ns-abc";
+ final String topic = "persistent://" + namespaceName + "/test-dynamicConfiguration-topic-auto-creation-"
+ + UUID.randomUUID();
+ try {
+ org.apache.pulsar.client.api.Producer<byte[]> producer = pulsarClient.newProducer()
+ .topic(topic)
+ .create();
+ } catch (Exception e) {
+ assertTrue(e instanceof PulsarClientException);
Review Comment:
```suggestion
assertTrue(e instanceof PulsarClientException.NotFoundException);
```
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1172139509
> Could you add `atMost` to other tests of `InactiveTopicDeleteTest` as well?
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1172836766
/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] Demogorgon314 commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1169439369
@codelipenghui @BewareMyPower Please help review this PR. The required test has already passed.
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1163910254
org.apache.pulsar.broker.admin.AdminApiTest ► testGetPartitionedStatsInternal：
Successfully run multiple times in my local environment：
org.apache.pulsar.broker.service.PersistentDispatcherFailoverConsumerTest ► testAddRemoveConsumer：
Successfully run multiple times in my local environment：
org.apache.pulsar.metadata.ZKSessionTest ► testSessionLost：
Runs successfully many times in my local environment, but occasionally fails:
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1172839390
@BewareMyPower Can you help me merge this branch to master?
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1155864681
Unit tests that fail to run also happen on the master branch, it doesn't look like it's caused by my commits, I'll try my best to take the time to fix them.
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164130597
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164479727
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164044881
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1163923404
/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] BewareMyPower commented on a diff in pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#discussion_r911681201
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/InactiveTopicDeleteTest.java:
##########
@@ -650,7 +650,6 @@ public void testBrokerDeleteInactivePartitionedTopicMetadataEnabled() throws Exc
admin.brokers()
.updateDynamicConfiguration("brokerDeleteInactivePartitionedTopicMetadataEnabled",
"true");
- Thread.sleep(2000);
Awaitility.await().untilAsserted(()->{
assertTrue(conf.isBrokerDeleteInactivePartitionedTopicMetadataEnabled());
});
Review Comment:
I mean, use `atMost` method to control the timeout explicitly. See https://www.javadoc.io/doc/org.awaitility/awaitility/3.0.0/org/awaitility/Awaitility.html
--
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] BewareMyPower commented on a diff in pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#discussion_r911678535
##########
conf/client.conf:
##########
@@ -22,12 +22,12 @@
# URL for Pulsar REST API (for admin operations)
# For TLS:
# webServiceUrl=https://localhost:8443/
-webServiceUrl=http://localhost:8080/
+webServiceUrl=http://localhost:8081/
# URL for Pulsar Binary Protocol (for produce and consume operations)
# For TLS:
# brokerServiceUrl=pulsar+ssl://localhost:6651/
-brokerServiceUrl=pulsar://localhost:6650/
+brokerServiceUrl=pulsar://localhost:6651,localhost:6652,localhost:6653/
Review Comment:
Please revert unnecessary changes.
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1163818865
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1163992402
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1172797496
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1171323487
/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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1164119997
/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] BewareMyPower commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1169760890
Please rebase to master to resolve conflicts.
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1171024277
/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] BewareMyPower commented on a diff in pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#discussion_r911586479
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/InactiveTopicDeleteTest.java:
##########
@@ -596,4 +597,64 @@ public void testHealthTopicInactiveNotClean() throws Exception {
Assert.assertTrue(V1Partitions.contains(healthCheckTopicV1));
Assert.assertTrue(V2Partitions.contains(healthCheckTopicV2));
}
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsEnabled() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsEnabled(true);
+ super.baseSetup();
+ admin.brokers().updateDynamicConfiguration("brokerDeleteInactiveTopicsEnabled", "false");
+ Awaitility.await().untilAsserted(()->{
+ assertFalse(conf.isBrokerDeleteInactiveTopicsEnabled());
+ });
+ }
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsFrequencySeconds() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsFrequencySeconds(30);
+ super.baseSetup();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactiveTopicsFrequencySeconds", "60");
+ Awaitility.await().untilAsserted(()->{
+ assertEquals(conf.getBrokerDeleteInactiveTopicsFrequencySeconds(), 60);
+ });
+ }
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsMaxInactiveDurationSeconds() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsMaxInactiveDurationSeconds(30);
+ super.baseSetup();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactiveTopicsMaxInactiveDurationSeconds", "60");
+ Awaitility.await().untilAsserted(()->{
+ assertEquals(conf.getBrokerDeleteInactiveTopicsMaxInactiveDurationSeconds(), 60);
+ });
+ }
+
+ @Test
+ public void testDynamicConfigurationBrokerDeleteInactiveTopicsMode() throws Exception {
+ conf.setBrokerDeleteInactiveTopicsMode (InactiveTopicDeleteMode.delete_when_no_subscriptions);
+ super.baseSetup();
+ String expect = InactiveTopicDeleteMode.delete_when_subscriptions_caught_up.toString();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactiveTopicsMode",
+ expect);
+ Awaitility.await().untilAsserted(()->{
+ assertEquals(conf.getBrokerDeleteInactiveTopicsMode().toString(), expect);
+ });
+ }
+
+ @Test
+ public void testBrokerDeleteInactivePartitionedTopicMetadataEnabled() throws Exception {
+ conf.setBrokerDeleteInactivePartitionedTopicMetadataEnabled(false);
+ super.baseSetup();
+ admin.brokers()
+ .updateDynamicConfiguration("brokerDeleteInactivePartitionedTopicMetadataEnabled",
+ "true");
+ Thread.sleep(2000);
+ Awaitility.await().untilAsserted(()->{
+ assertTrue(conf.isBrokerDeleteInactivePartitionedTopicMetadataEnabled());
+ });
+ }
Review Comment:
Don't use `Thread.sleep`, use `Awaitility.await().atMost(/* ... */)`.
I noticed some existing tests might also use `Thread.sleep`, but it's not good. For new tests, it's better not to sleep.
--
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] Radiancebobo commented on pull request #16023: [fix][admin]allowAutoTopicCreation and allowAutoTopicCreationType etc should be dynamically configured
Posted by GitBox <gi...@apache.org>.
Radiancebobo commented on PR #16023:
URL: https://github.com/apache/pulsar/pull/16023#issuecomment-1153450218
> The change looks good. Could you please add a UT for the new change?
>
> Here is an example PR #13121 which added `subscriptionTypesEnabled` dynamic configuration support.
OK, I will supplement the corresponding unit tests as soon as possible
--
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