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/07/25 13:22:13 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
poorbarcode opened a new pull request, #16781:
URL: https://github.com/apache/pulsar/pull/16781
Master Issue: #16135
### Motivation
see #16135
### Modifications
I will complete proposal #16135 with these pull requests( *current pull request is the step-2* ):
1. Add A Jackson component `DynamicSkipUnknownPropertyHandler` that supports dynamically policy of rejecting unknown parameters. Prints warn log when it receives an unknown parameter.
2. Dynamic configuration.
### Documentation
- [ ] `doc-required`
- [x] `doc-not-needed`
- [ ] `doc`
- [ ] `doc-complete`
--
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] Jason918 commented on a diff in pull request #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #16781:
URL: https://github.com/apache/pulsar/pull/16781#discussion_r929455391
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -1487,6 +1487,13 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private double httpRequestsMaxPerSecond = 100.0;
+ @FieldContext(
+ category = CATEGORY_HTTP,
+ dynamic = true,
+ doc = "Admin API fail on unknown request parameter in request-body. see PIP-178. Default false."
Review Comment:
It's PIP-179 in the title of this PR. which is right?
--
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 #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16781:
URL: https://github.com/apache/pulsar/pull/16781#issuecomment-1194857483
@poorbarcode Please provide a correct 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] poorbarcode commented on a diff in pull request #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16781:
URL: https://github.com/apache/pulsar/pull/16781#discussion_r929472080
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminRestTest.java:
##########
@@ -60,15 +65,36 @@ public void testRejectUnknownEntityProperties() throws Exception{
Response response = target.request(MediaType.APPLICATION_JSON_TYPE).buildPost(Entity.json(data)).invoke();
Assert.assertTrue(response.getStatus() / 200 == 1);
// Enabled feature, bad request response.
- pulsar.getWebService().getSharedUnknownPropertyHandler().setSkipUnknownProperty(false);
+ admin.brokers().updateDynamicConfiguration("httpRequestsFailOnUnknownPropertiesEnabled", "true");
+ Awaitility.await().atMost(2, TimeUnit.SECONDS).until(
+ () -> !pulsar.getWebService().getSharedUnknownPropertyHandler().isSkipUnknownProperty()
+ );
response = target.request(MediaType.APPLICATION_JSON_TYPE).buildPost(Entity.json(data)).invoke();
+ Assert.assertEquals(MediaType.valueOf(MediaType.TEXT_PLAIN), response.getMediaType());
+ String responseBody = parseResponseEntity(response.getEntity());
+ Assert.assertEquals(responseBody, "Unknown property retention_time_in_minutes, perhaps you want to use"
+ + " one of these: [retentionSizeInMB, retentionTimeInMinutes]");
Assert.assertEquals(response.getStatus(), 400);
// Disabled feature, response success.
- pulsar.getWebService().getSharedUnknownPropertyHandler().setSkipUnknownProperty(true);
+ admin.brokers().updateDynamicConfiguration("httpRequestsFailOnUnknownPropertiesEnabled", "false");
+ Awaitility.await().atMost(2, TimeUnit.SECONDS).until(
+ () -> pulsar.getWebService().getSharedUnknownPropertyHandler().isSkipUnknownProperty()
+ );
response = target.request(MediaType.APPLICATION_JSON_TYPE).buildPost(Entity.json(data)).invoke();
Assert.assertTrue(response.getStatus() / 200 == 1);
}
+ private String parseResponseEntity(Object entity) throws Exception {
+ InputStream in = (InputStream) entity;
+ BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(in));
Review Comment:
Already fixed. Thanks.
The newly submitted: input stream has closed by `response.close`
--
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 #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16781:
URL: https://github.com/apache/pulsar/pull/16781#discussion_r929457048
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -1487,6 +1487,13 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private double httpRequestsMaxPerSecond = 100.0;
+ @FieldContext(
+ category = CATEGORY_HTTP,
+ dynamic = true,
+ doc = "Admin API fail on unknown request parameter in request-body. see PIP-178. Default false."
Review Comment:
Already fixed. Thanks
--
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 diff in pull request #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #16781:
URL: https://github.com/apache/pulsar/pull/16781#discussion_r929467901
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminRestTest.java:
##########
@@ -60,15 +65,36 @@ public void testRejectUnknownEntityProperties() throws Exception{
Response response = target.request(MediaType.APPLICATION_JSON_TYPE).buildPost(Entity.json(data)).invoke();
Assert.assertTrue(response.getStatus() / 200 == 1);
// Enabled feature, bad request response.
- pulsar.getWebService().getSharedUnknownPropertyHandler().setSkipUnknownProperty(false);
+ admin.brokers().updateDynamicConfiguration("httpRequestsFailOnUnknownPropertiesEnabled", "true");
+ Awaitility.await().atMost(2, TimeUnit.SECONDS).until(
+ () -> !pulsar.getWebService().getSharedUnknownPropertyHandler().isSkipUnknownProperty()
+ );
response = target.request(MediaType.APPLICATION_JSON_TYPE).buildPost(Entity.json(data)).invoke();
+ Assert.assertEquals(MediaType.valueOf(MediaType.TEXT_PLAIN), response.getMediaType());
+ String responseBody = parseResponseEntity(response.getEntity());
+ Assert.assertEquals(responseBody, "Unknown property retention_time_in_minutes, perhaps you want to use"
+ + " one of these: [retentionSizeInMB, retentionTimeInMinutes]");
Assert.assertEquals(response.getStatus(), 400);
// Disabled feature, response success.
- pulsar.getWebService().getSharedUnknownPropertyHandler().setSkipUnknownProperty(true);
+ admin.brokers().updateDynamicConfiguration("httpRequestsFailOnUnknownPropertiesEnabled", "false");
+ Awaitility.await().atMost(2, TimeUnit.SECONDS).until(
+ () -> pulsar.getWebService().getSharedUnknownPropertyHandler().isSkipUnknownProperty()
+ );
response = target.request(MediaType.APPLICATION_JSON_TYPE).buildPost(Entity.json(data)).invoke();
Assert.assertTrue(response.getStatus() / 200 == 1);
}
+ private String parseResponseEntity(Object entity) throws Exception {
+ InputStream in = (InputStream) entity;
+ BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(in));
Review Comment:
Need to close the `bufferedReader`
--
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- merged pull request #16781: [improve] [admin] [PIP-179] Dynamic configuration for check unknown request parameters
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #16781:
URL: https://github.com/apache/pulsar/pull/16781
--
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