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/09/09 16:38:12 UTC
[GitHub] [pulsar] shibd opened a new pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
shibd opened a new pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986
#11923
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r707513486
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (checkAllowAutoCreation) {
Review comment:
Yes, Validate `WRITE` permission only when `checkAllowAutoCreation = true`. I added this unit test case.
--
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] syhily commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
syhily commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-961164123
@BewareMyPower This issue block the pulsar-flink-connector. Can you help me review this MR?
--
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] Anonymitaet commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r743303124
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
+When topic auto-creation is enabled by broker, you can set param `checkAllowAutoCreation = true` to auto create topic.
Review comment:
```suggestion
When enabling topic auto-creation, you can set the parameter `checkAllowAutoCreation = true` to automatically create a topic.
```
do you mean this? if so, please help update all occurrences.
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
Review comment:
Here we have `<!--REST API-->` and `<!--Java-->` methods, do we have `<!--pulsar-admin-->` method?
--
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 #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-924070251
> but I think this pr is introducing a breaking change that now producer will need admin permission to be able to produce if using HttpLookupService, since when creating producer it'll get topic metadata first, which now validate `WRITE` permission on `TOPIC`, while current authz check is coarse-grained so it just check if role has admin permission
> that's why the cpp test is failing
> @codelipenghui @hangc0276 @eolivelli any thought on this?
I recently filed this issue for a similar problem
https://github.com/apache/pulsar/issues/11945
btw you are right, this is a breaking change.
I believe that we should relax the authz checks if you only want to get the list of partitions, you do not need "admin" access
--
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] MarvinCai commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-924017682
but I think this pr is introducing a breaking change that now producer will need admin permission to be able to produce if using HttpLookupService, since when creating producer it'll get topic metadata first, which now validate `WRITE` permission on `TOPIC`, while current authz check is coarse-grained so it just check if role has admin permission
that's why the cpp test is failing
@codelipenghui @hangc0276 @eolivelli any thought on this?
--
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] Anonymitaet commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-916789796
@shibd Thanks for your contribution. Please do not forget to add docs accordingly to allow users to know your great code changes. And you can ping me to review the docs, 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] MarvinCai commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r706825383
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (checkAllowAutoCreation) {
Review comment:
this method is `getPartitionedMetadata`, should only need `READ` permission 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] shibd edited a comment on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd edited a comment on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-919811581
> cpp test is broke with this patch, can you take a look:
Thanks reply, I fix it. Can you take look?
--
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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-929738121
--
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] shibd removed a comment on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd removed a comment on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-925463056
@eolivelli
--
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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-929752475
@MarvinCai @eolivelli hi, I fixed cpp unit test, can you help me review it?
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r711977080
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (authoritative && checkAllowAutoCreation) {
Review comment:
sorry, I got it wrong. I changed it back.
--
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] MarvinCai commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-922269116
/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] Anonymitaet commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r743303124
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
+When topic auto-creation is enabled by broker, you can set param `checkAllowAutoCreation = true` to auto create topic.
Review comment:
```suggestion
When enabling topic auto-creation, you can set the parameter `checkAllowAutoCreation = true` to automatically create a topic.
```
do you mean this? if so, please help update all occurrences.
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
Review comment:
Here we have `<!--REST API-->` and `<!--Java-->` methods, do we have `<!--pulsar-admin-->` method?
--
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 #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-924071114
> @shibd the cpp test should be able to fixed by adding `-r "token-principal"` to line 110 of `pulsar-test-service-start.sh`
@MarvinCai can you please send a patch ?
--
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] gaoran10 commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r706854463
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -389,6 +392,27 @@ public void testNonPartitionedTopics() {
0);
}
+ @Test
+ public void testGetPartitionedMetadata() {
+
+ String testLocalTopicName = "test-topic";
+
+ // 1) Auto create non-partitioned topic.
+ PartitionedTopicMetadata partitionedMetadata = persistentTopics.getPartitionedMetadata(testTenant, testNamespace, testLocalTopicName, true, true);
+ Assert.assertEquals(0, partitionedMetadata.partitions);
+
+ // 2. Add validate policy assert.
+ doThrow(new RestException(Response.Status.FORBIDDEN, "mock message"))
+ .when(persistentTopics).validateTopicPolicyOperation(persistentTopics.topicName, PolicyName.PARTITION, PolicyOperation.WRITE);
+ try {
+ persistentTopics.getPartitionedMetadata(testTenant, testNamespace, testLocalTopicName, true, true);
+ Assert.fail("Should fail validation on policy exception");
+ } catch (RestException e) {
+ Assert.assertEquals(Response.Status.FORBIDDEN.getStatusCode(), e.getResponse().getStatus());
+ }
+
Review comment:
Could we add tests to when the param `checkAllowAutoCreation` is `false`?
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r706847552
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (checkAllowAutoCreation) {
Review comment:
This method can auto create topic, so need validate `WRITE` permission.
--
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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-916612336
@Anonymitaet Thanks,I readjusted the PR description.
--
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] syhily edited a comment on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
syhily edited a comment on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-961164123
@BewareMyPower This issue blocks the pulsar-flink-connector. Can you help me review this MR?
--
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 #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-961190700
This PR is too late to review. I'm afraid it could not be included in Pulsar 2.9.0 now.
This PR adds a new overridden of `getPartitionedTopicMetadataAsync` to `Topics`, but it doesn't affect the existing APIs. Is it suitable to add this PR to 2.8.2? @315157973
--
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] Anonymitaet commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-916599263
@shibd Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? 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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-929738121
/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] shibd closed pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd closed pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986
--
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] MarvinCai commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r707337365
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (checkAllowAutoCreation) {
Review comment:
then what if user with only read permission just want to get meta data? they should be able to fetch the partition meta 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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-925463056
@eolivelli
--
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] shibd closed pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd closed pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r743324351
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
Review comment:
No, pulsar-admin does not have this method
--
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] shibd closed pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd closed pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986
--
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] MarvinCai commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-919656721
/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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-917902370
@Anonymitaet I added docs, Can you help me review it?
--
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 #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-961196210
@MarvinCai @gaoran10 PTAL again. I think the comments are all addressed 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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-916261417
@codelipenghui Can you help me review it?
--
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] MarvinCai commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-924012763
@shibd the cpp test should be able to fixed by adding `-r "token-principal"` to line 110 of `pulsar-test-service-start.sh`
--
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] MarvinCai commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r711842334
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (authoritative && checkAllowAutoCreation) {
Review comment:
I don't think we should include `authoritive` here? https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/LookupOptions.java#L28-L31
https://github.com/apache/pulsar/blame/master/site2/website/versioned_docs/version-2.8.1/developing-binary-protocol.md#L478-L480
--
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] MarvinCai commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-924854230
I think it's safer to just paste the patch as it's just single line change:
```
diff --git a/pulsar-client-cpp/pulsar-test-service-start.sh b/pulsar-client-cpp/pulsar-test-service-start.sh
index 248d628b9c2..2b656ddf6dd 100755
--- a/pulsar-client-cpp/pulsar-test-service-start.sh
+++ b/pulsar-client-cpp/pulsar-test-service-start.sh
@@ -107,7 +107,7 @@ $PULSAR_DIR/bin/pulsar-admin namespaces grant-permission public/default-4 \
$PULSAR_DIR/bin/pulsar-admin namespaces set-encryption-required public/default-4 -e
# Create "private" tenant
-$PULSAR_DIR/bin/pulsar-admin tenants create private -r "" -c "standalone"
+$PULSAR_DIR/bin/pulsar-admin tenants create private -r "token-principal" -c "standalone"
# Create "private/auth" with required authentication
$PULSAR_DIR/bin/pulsar-admin namespaces create private/auth --clusters standalone
```
--
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 #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-924892284
@MarvinCai
in order to apply the patch we need a PR, this is our merge workflow.
if I create the PR I will be listed as author, but you are the actual author of the patch, so it would be very better that you post the PR
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r708394838
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -786,6 +786,9 @@ public PartitionedTopicMetadata getPartitionedMetadata(
@ApiParam(value = "Is check configuration required to automatically create topic")
@QueryParam("checkAllowAutoCreation") @DefaultValue("false") boolean checkAllowAutoCreation) {
validateTopicName(tenant, namespace, encodedTopic);
+ if (checkAllowAutoCreation) {
Review comment:
@MarvinCai Hi, Do you have any more 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] shibd commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-919811581
> cpp test is broke with this patch, can you take a look:
Thank reply, I fix it. Can you take look?
--
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] MarvinCai commented on pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#issuecomment-919665055
cpp test is broke with this patch, can you take a look:
```
2021-09-15 03:10:19.336 INFO [140681233118976] HTTPLookupService:238 | Curl Lookup Request sent for http://localhost:8080/admin/v2/persistent/private/auth/test-token-http/partitions?checkAllowAutoCreation=true
2021-09-15 03:10:19.344 ERROR [140681233118976] HTTPLookupService:263 | Response failed for url http://localhost:8080/admin/v2/persistent/private/auth/test-token-http/partitions?checkAllowAutoCreation=true. Error Code 22
2021-09-15 03:10:19.344 ERROR [140681233118976] ClientImpl:188 | Error Checking/Getting Partition Metadata while creating producer on persistent://private/auth/test-token-http -- ConnectError
/pulsar/pulsar-client-cpp/tests/AuthTokenTest.cc:128: Failure
```
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r743324351
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
Review comment:
No, pulsar-admin does not have this method
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
Review comment:
No, pulsar-admin does not have this method
--
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] Anonymitaet commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r743303124
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
+When topic auto-creation is enabled by broker, you can set param `checkAllowAutoCreation = true` to auto create topic.
Review comment:
```suggestion
When enabling topic auto-creation, you can set the parameter `checkAllowAutoCreation = true` to automatically create a topic.
```
do you mean this? if so, please help update all occurrences.
##########
File path: site2/docs/admin-api-topics.md
##########
@@ -1138,12 +1138,18 @@ $ pulsar-admin topics get-partitioned-topic-metadata \
```
<!--REST API-->
Review comment:
Here we have `<!--REST API-->` and `<!--Java-->` methods, do we have `<!--pulsar-admin-->` method?
--
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] shibd commented on a change in pull request #11986: [pulsar admin] getPartitionedTopicMetada method support setting auto create topic
Posted by GitBox <gi...@apache.org>.
shibd commented on a change in pull request #11986:
URL: https://github.com/apache/pulsar/pull/11986#discussion_r707058579
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -389,6 +392,27 @@ public void testNonPartitionedTopics() {
0);
}
+ @Test
+ public void testGetPartitionedMetadata() {
+
+ String testLocalTopicName = "test-topic";
+
+ // 1) Auto create non-partitioned topic.
+ PartitionedTopicMetadata partitionedMetadata = persistentTopics.getPartitionedMetadata(testTenant, testNamespace, testLocalTopicName, true, true);
+ Assert.assertEquals(0, partitionedMetadata.partitions);
+
+ // 2. Add validate policy assert.
+ doThrow(new RestException(Response.Status.FORBIDDEN, "mock message"))
+ .when(persistentTopics).validateTopicPolicyOperation(persistentTopics.topicName, PolicyName.PARTITION, PolicyOperation.WRITE);
+ try {
+ persistentTopics.getPartitionedMetadata(testTenant, testNamespace, testLocalTopicName, true, true);
+ Assert.fail("Should fail validation on policy exception");
+ } catch (RestException e) {
+ Assert.assertEquals(Response.Status.FORBIDDEN.getStatusCode(), e.getResponse().getStatus());
+ }
+
Review comment:
Good idea, I 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