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/12/21 03:41:12 UTC
[GitHub] [pulsar] nodece opened a new pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
nodece opened a new pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420
[Broker] Fix create the dynamic configuration resource if not exist
### Motivation
When request the `DELETE: /admin/brokers/configuration/dispatcherMinReadBatchSize`, which return the`org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException`. The Pulsar does not create the dynamic configuration resource if it does not exist, this is incorrect behavior.
### Modifications
- Add check dynamic configuration resource on `updateDynamicServiceConfiguration()`
### Documentation
Need to update docs?
- [x] `no-need-doc`
Signed-off-by: Zixuan Liu <no...@gmail.com>
--
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 #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000059566
IMHO, We can simply catch the `MetadataStoreException.NotFoundException` in `BrokersBase#deleteDynamicConfigurationOnZk` when the dynamicConfigurationResources not 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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-998896521
@codelipenghui test has been 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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000061648
@Demogorgon314 Pulsar did not create this resource in metadata, so throw the `MetadataStoreException.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] codelipenghui merged pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420
--
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] nodece removed a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-998483065
/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] nodece removed a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999251127
--
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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999251127
/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] nodece removed a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-998523716
/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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999326963
/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 a change in pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#discussion_r774423565
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -181,7 +181,7 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
public Map<String, String> getAllDynamicConfigurations() throws Exception {
validateSuperUserAccess();
try {
- return dynamicConfigurationResources().getDynamicConfiguration();
+ return dynamicConfigurationResources().getDynamicConfiguration().orElseGet(Maps::newHashMap);
Review comment:
Better be use `Collections.emptyMap()`, it is `EMPTY_MAP`.
--
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] nodece removed a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999531719
--
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] nodece edited a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000069320
@Demogorgon314 This is a bug. The dynamic configuration resource path never is deleted on metadata, when we delete a dynamic configuration value, we just remove the value stored in the path.
You can checkout this code: https://github.com/apache/pulsar/blob/v2.8.0/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2092, this is Pulsar 2.8 behavior.
--
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] nodece removed a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999223429
/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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999531719
/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] nodece removed a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999531420
--
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 change in pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#discussion_r774362146
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/DynamicConfigurationResources.java
##########
@@ -41,7 +41,7 @@ public DynamicConfigurationResources(MetadataStore store, int operationTimeoutSe
}
public Map<String, String> getDynamicConfiguration() throws MetadataStoreException {
- return get(BROKER_SERVICE_CONFIGURATION_PATH).orElse(Collections.emptyMap());
+ return get(BROKER_SERVICE_CONFIGURATION_PATH).orElse(null);
Review comment:
It changes the original behavior. This method is also used in `BrokersBase#getAllDynamicConfigurations`. Maybe it doesn't affect, but I think it's better to keep the current semantic unchanged.
I'd prefer to return an optional object for this method so that the synchronous method `getDynamicConfiguration` will be consistent with the asynchronous method `getDynamicConfigurationAsync`.
```java
public Optional<Map<String, String>> getDynamicConfiguration() throws MetadataStoreException {
return get(BROKER_SERVICE_CONFIGURATION_PATH);
}
```
Then in `BrokerService#updateDynamicServiceConfiguration`, we can call it like
```java
configCache = pulsar().getPulsarResources().getDynamicConfigResources().getDynamicConfiguration();
// create dynamic-config if not exist.
if (!configCache.isPresent()) {
```
It's more clear than null check. And in `BrokerBase#getAllDynamicConfigurations`, we can simply keep the original implementation like
```java
return dynamicConfigurationResources().getDynamicConfiguration().orElse(Collections.emptyMap());
```
--
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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999531420
/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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000069320
@Demogorgon314 This is a bug. The dynamic configuration resource path never is deleted on metadata, when we delete a dynamic configuration value, we just remove the value stored in the path.
--
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] nodece edited a comment on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000061648
@Demogorgon314 Pulsar did not create this resource in metadata, so throw the `MetadataStoreException.NotFoundException`, when we update this configuration it is always successful.
--
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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-998483065
/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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-998523716
/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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999602884
/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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999328214
/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 change in pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#discussion_r774338403
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiBrokerTest.java
##########
@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.admin;
+
+import static org.junit.Assert.fail;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import java.util.Map;
+import javax.ws.rs.core.Response;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+@Slf4j
+@Test(groups = "broker")
+public class AdminApiBrokerTest extends MockedPulsarServiceBaseTest {
Review comment:
Could you use another name? It's more like an `AdminApiDynamicConfigurationsTest`.
--
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] nodece commented on pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-999223429
/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 #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000063625
> Pulsar did not create this resource in metadata, so throw the `MetadataStoreException.NotFoundException`.
Exactly. So we can catch the `MetadataStoreException.NotFoundException` like this.
```java
private synchronized void deleteDynamicConfigurationOnZk(String configName) {
try {
// ...
} catch (RestException re) {
throw re;
} catch (MetadataStoreException.NotFoundException ex) {
LOG.info("xxx");
} catch (Exception ie) {
LOG.error("[{}] Failed to update configuration {}, {}", clientAppId(), configName, ie.getMessage(), ie);
throw new RestException(ie);
}
}
```
--
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 #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#issuecomment-1000076810
The problem is that should or not be lazy create the config path. Just an idea. I think it is better to create the path when we have dynamic config in there. Of course, the current solution is no problem.
--
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] Technoboy- commented on a change in pull request #13420: [Broker] Fix create the dynamic configuration resource if not exist
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #13420:
URL: https://github.com/apache/pulsar/pull/13420#discussion_r774410350
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -181,7 +181,7 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
public Map<String, String> getAllDynamicConfigurations() throws Exception {
validateSuperUserAccess();
try {
- return dynamicConfigurationResources().getDynamicConfiguration();
+ return dynamicConfigurationResources().getDynamicConfiguration().orElse(Maps.newHashMap());
Review comment:
orElse -> orElseGet.
--
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