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/11/15 16:27:21 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #12818: PIP 110: Support Topic metadata
Technoboy- opened a new pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818
Fixes #12629
## Motivation
The original discussion mail :
https://lists.apache.org/thread/m9dkhq1fs6stsdwh78h84fsl5hs5v67f
Introduce the ability to store metadata about topics.
This would be very useful as with metadata you could add labels and other
pieces of information that would allow defining the purpose of a topic,
custom application-level properties.
This feature will allow application-level diagnostic tools and maintenance
tools to not need external databases to store such metadata.
Imagine that we could add a simple key value map (String keys and String
values) to the topic.
These metadata could be set during topic creation and also updated.
## API Changes
We need to add a map type field called `metadata` to the below methods signature:
- PersistentTopics#createPartitionedTopic
```
PersistentTopics#createPartitionedTopic(AsyncResponse asyncResponse, String tenant, String namespace,String encodedTopic,int numPartitions,boolean createLocalTopicOnly, Map<String, String> metadata);
```
- PersistentTopics#createNonPartitionedTopic
```
PersistentTopics#createNonPartitionedTopic(String tenant, String namespace, String encodedTopic, boolean authoritative, Map<String, String> metadata);
```
## Implementation
- For PartitionedTopic
We will put these metadata to the PartitionedTopicMetadata, serialized to store in configurationMetadataStore, like partitioned-topic partitions.
```
public class PartitionedTopicMetadata {
/* Number of partitions for the topic */
public int partitions;
/* Topic metadata */
public Map<String, String> metadata;
```
- For NonPartitionedTopic
We will store the metadata to ML(ManagedLedger) which already supports storing custom properties.
```
ManagedLedger#setProperties(Map<String, String> properties)
```
## Compatibility
The proposal will not introduce any compatibility issues.
## Tests Plan
Unit tests & integration tests
### Documentation
- [ x ] `no-need-doc`
--
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] yuruguo commented on a change in pull request #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749886972
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -368,9 +368,10 @@ public void revokePermissions(String topic, String role) throws PulsarAdminExcep
}
@Override
- public void createPartitionedTopic(String topic, int numPartitions) throws PulsarAdminException {
+ public void createPartitionedTopic(String topic, int numPartitions, Map<String, String> metadata)
+ throws PulsarAdminException {
try {
- createPartitionedTopicAsync(topic, numPartitions).get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
+ createPartitionedTopicAsync(topic, numPartitions, metadata).get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
Review comment:
suggest to use new method to pack this piece
https://github.com/apache/pulsar/blob/9994614173205abd075fcc670396cebd71227047/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java#L290
such as
https://github.com/apache/pulsar/blob/9994614173205abd075fcc670396cebd71227047/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/NamespacesImpl.java#L244-L246
--
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] lhotari commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r798663579
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/PersistentTopics.java
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.v3;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.Encoded;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.container.AsyncResponse;
+import javax.ws.rs.container.Suspended;
+import javax.ws.rs.core.MediaType;
+import org.apache.pulsar.broker.admin.impl.PersistentTopicsBase;
+import org.apache.pulsar.common.partition.PartitionedTopicMetadata;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ */
+@Path("/persistent")
+@Produces(MediaType.APPLICATION_JSON)
+@Api(value = "/persistent", description = "Persistent topic admin apis", tags = "persistent topic")
+public class PersistentTopics extends PersistentTopicsBase {
Review comment:
> Because we have changed the method from createPartitionedTopic(partitions) to createPartitionedTopic(partitionedTopicMetadata)
It should be possible to handle that in a backwards compatible way without breaking the API. It's better to revert the addition of a new API version and instead revisit the solution in a proper way.
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r786710453
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
##########
@@ -173,6 +173,34 @@ public void createPartitionedTopic(
}
}
+ @PUT
+ @Path("/{property}/{cluster}/{namespace}/{topic}/partitions/properties")
+ @ApiOperation(hidden = true, value = "Create a partitioned topic.",
Review comment:
Ok, I will remove 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] Technoboy- commented on pull request #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#issuecomment-1015371312
> Do we have a way to get the properties? Looks like the PR just supports set properties, but is not able to get the properties.
These new properties are put in PartitionedTopicMetadata. So we can get it from `getPartitionedMetadata`
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r789408110
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -215,7 +215,8 @@
prepareDynamicConfigurationMap();
private final ConcurrentOpenHashMap<String, Consumer<?>> configRegisteredListeners;
- private final ConcurrentLinkedQueue<Pair<String, CompletableFuture<Optional<Topic>>>> pendingTopicLoadingQueue;
+ private final ConcurrentLinkedQueue<Triple<String, CompletableFuture<Optional<Topic>>, Map<String, String>>>
Review comment:
Ok, good suggestion.
--
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 commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787606867
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
Interesting...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r790549736
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -1239,6 +1251,13 @@ private WebTarget namespacePath(String domain, NamespaceName namespace, String..
return namespacePath;
}
+ private WebTarget topicPath(TopicName topic, Map<String, String> metadata, String... parts) {
+ final WebTarget base = metadata != null ? adminV3Topics : (topic.isV2() ? adminV2Topics : adminTopics);
Review comment:
so this is where we select the "version" of the API for the topics.
can you please add a comment here in order to give more context ?
in the future we will have to use the v3 API for new features.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787446962
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
For there exists one param in this method before, we can't add a new param to this method because the body value only supports one param.
--
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 pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#issuecomment-1016214282
> BTW, could you check [#12629 (comment)](https://github.com/apache/pulsar/issues/12629#issuecomment-1003723222)? I have the same concern that for a partitioned topic, what's the behavior when an individual partition's properties was set?
>
> For example, I ran following commands:
>
> ```shell
> curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'
> curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx-partition-0 -X DELETE
> curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx-partition-0 -X PUT -H 'Content-Type: application/json' -d '{"k3":"v3"}'
> ```
>
> 1. Create a partitioned topic `xxx` with 3 partitions and properties `k1 -> v1, k2 -> v2`.
> 2. Delete the 1st partition (`xxx-partition-0`).
> 3. Create the 1st partition with properties `k3 -> v3`.
>
> The 3rd step works well but I think **it should be prevented**. /cc @eolivelli Is it what you concerned?
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] codelipenghui commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787614545
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
I have tried `curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'`
Got 400 at the broker side:
```
2022-01-19T18:41:56,584+0800 [pulsar-web-59-12] INFO org.eclipse.jetty.server.RequestLog - 127.0.0.1 - - [19/Jan/2022:18:41:56 +0800] "PUT /admin/v2/persistent/public/default/xxx/partitions HTTP/1.1" 400 212 "-" "curl/7.77.0" 2
```
Got exception at the client-side
```
Cannot deserialize value of type `int` from Object value (token `JsonToken.START_OBJECT`)
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 1]%
```
--
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 commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787616684
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
If we can't avoid breaking compatibility, we can add a new endpoint in `v3` REST APIs
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749947366
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java
##########
@@ -77,6 +77,7 @@
private int newEntriesCheckDelayInMillis = 10;
private Clock clock = Clock.systemUTC();
private ManagedLedgerInterceptor managedLedgerInterceptor;
+ private Map<String, String> topicMetadata;
Review comment:
Discuss with @codelipenghui , add this field to ledger config can decrease zk op.
--
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 change in pull request #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749872569
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java
##########
@@ -77,6 +77,7 @@
private int newEntriesCheckDelayInMillis = 10;
private Clock clock = Clock.systemUTC();
private ManagedLedgerInterceptor managedLedgerInterceptor;
+ private Map<String, String> topicMetadata;
Review comment:
In my understanding, this `topicMetadata ` is for meta info of PartitionedTopic. Does it have anything to do with ManagedLedger？
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -383,10 +385,28 @@ void run() throws PulsarAdminException {
"--partitions" }, description = "Number of partitions for the topic", required = true)
private int numPartitions;
+ @Parameter(names = {"--topic-metadata", "-tm"}, description = "key value pair properties(a=a,b=b,c=c)",
+ required = true)
Review comment:
required = true? IMHO, this should be optional?
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749886018
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java
##########
@@ -77,6 +77,7 @@
private int newEntriesCheckDelayInMillis = 10;
private Clock clock = Clock.systemUTC();
private ManagedLedgerInterceptor managedLedgerInterceptor;
+ private Map<String, String> topicMetadata;
Review comment:
Yes，we may don't need to add this field.
Before, I store this metadata to the ledger properties in ledger#openLedgerComplete. But consideration of atomic, add this field. We will discuss this later. This pr will take some time to be merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r790547963
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/PersistentTopics.java
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.v3;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.Encoded;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.container.AsyncResponse;
+import javax.ws.rs.container.Suspended;
+import javax.ws.rs.core.MediaType;
+import org.apache.pulsar.broker.admin.impl.PersistentTopicsBase;
+import org.apache.pulsar.common.partition.PartitionedTopicMetadata;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ */
+@Path("/persistent")
+@Produces(MediaType.APPLICATION_JSON)
+@Api(value = "/persistent", description = "Persistent topic admin apis", tags = "persistent topic")
+public class PersistentTopics extends PersistentTopicsBase {
Review comment:
makes sense to me.
so when you use the "new" pulsar-admin (2.10) with an old broker
how can we deal with compatibility ?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r789499904
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/PersistentTopics.java
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.v3;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.Encoded;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.container.AsyncResponse;
+import javax.ws.rs.container.Suspended;
+import javax.ws.rs.core.MediaType;
+import org.apache.pulsar.broker.admin.impl.PersistentTopicsBase;
+import org.apache.pulsar.common.partition.PartitionedTopicMetadata;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ */
+@Path("/persistent")
+@Produces(MediaType.APPLICATION_JSON)
+@Api(value = "/persistent", description = "Persistent topic admin apis", tags = "persistent topic")
+public class PersistentTopics extends PersistentTopicsBase {
Review comment:
Because we have changed the method from `createPartitionedTopic(partitions)` to `createPartitionedTopic(partitionedTopicMetadata)`
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787463201
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -215,7 +215,8 @@
prepareDynamicConfigurationMap();
private final ConcurrentOpenHashMap<String, Consumer<?>> configRegisteredListeners;
- private final ConcurrentLinkedQueue<Pair<String, CompletableFuture<Optional<Topic>>>> pendingTopicLoadingQueue;
+ private final ConcurrentLinkedQueue<Triple<String, CompletableFuture<Optional<Topic>>, Map<String, String>>>
Review comment:
I think using `Triple` harms the code readability. I'd rather like adding a simple class
```java
@AllArgsConstructor
private static class TopicLoadingContext {
private final String name;
private final CompletableFuture<Optional<Topic>> topicFuture;
private final Map<String, String> properties;
}
```
We don't need the `Triple` here because we don't use it as a key of `Map` so we don't need the `compareTo`, `equals` and `hashCode` methods that `Triple` has implemented by default.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787627405
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
Did you compile the latest code? Here are the test input from my local env:
```bash
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3,"properties":{"k1":"v1","k2":"v2"}} $ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X DELETE
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k3":"v3"}}'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3,"properties":{"k3":"v3"}}
```
If you used the original code, since the body must be an integer, the body `{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}` cannot be recognized.
```bash
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '3'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3} $ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3"}'
Cannot deserialize value of type `int` from Object value (token `JsonToken.START_OBJECT`)
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 1]
```
But I don't think it breaks the backward compatibility. This PR makes the REST API available for more input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r789427825
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/PersistentTopics.java
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.v3;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.Encoded;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.container.AsyncResponse;
+import javax.ws.rs.container.Suspended;
+import javax.ws.rs.core.MediaType;
+import org.apache.pulsar.broker.admin.impl.PersistentTopicsBase;
+import org.apache.pulsar.common.partition.PartitionedTopicMetadata;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ */
+@Path("/persistent")
+@Produces(MediaType.APPLICATION_JSON)
+@Api(value = "/persistent", description = "Persistent topic admin apis", tags = "persistent topic")
+public class PersistentTopics extends PersistentTopicsBase {
Review comment:
Why do we need this new package?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#issuecomment-1016206450
BTW, could you check https://github.com/apache/pulsar/issues/12629#issuecomment-1003723222? I have the same concern that for a partitioned topic, what's the behavior when an individual partition's properties was set?
For example, I ran following commands:
```bash
curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'
curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx-partition-0 -X DELETE
curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx-partition-0 -X PUT -H 'Content-Type: application/json' -d '{"k3":"v3"}'
```
1. Create a partitioned topic `xxx` with 3 partitions and properties `k1 -> v1, k2 -> v2`.
2. Delete the 1st partition (`xxx-partition-0`).
3. Create the 1st partition with properties `k3 -> v3`.
The 3rd step works well but I think **it should be prevented**. /cc @eolivelli Is it what you concerned?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r789378747
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
Got it. I think there is no good way for this compatibility. We can only guarantee **New Broker is compatible with Old Client**
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r790717219
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -1239,6 +1251,13 @@ private WebTarget namespacePath(String domain, NamespaceName namespace, String..
return namespacePath;
}
+ private WebTarget topicPath(TopicName topic, Map<String, String> metadata, String... parts) {
+ final WebTarget base = metadata != null ? adminV3Topics : (topic.isV2() ? adminV2Topics : adminTopics);
Review comment:
done, 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 change in pull request #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749881580
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -383,10 +385,28 @@ void run() throws PulsarAdminException {
"--partitions" }, description = "Number of partitions for the topic", required = true)
private int numPartitions;
+ @Parameter(names = {"--topic-metadata", "-tm"}, description = "key value pair properties(a=a,b=b,c=c)",
+ required = true)
Review comment:
Removed
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r789430652
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/PersistentTopics.java
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.v3;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.Encoded;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.container.AsyncResponse;
+import javax.ws.rs.container.Suspended;
+import javax.ws.rs.core.MediaType;
+import org.apache.pulsar.broker.admin.impl.PersistentTopicsBase;
+import org.apache.pulsar.common.partition.PartitionedTopicMetadata;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ */
+@Path("/persistent")
+@Produces(MediaType.APPLICATION_JSON)
+@Api(value = "/persistent", description = "Persistent topic admin apis", tags = "persistent topic")
+public class PersistentTopics extends PersistentTopicsBase {
Review comment:
You can see the discussion in https://github.com/apache/pulsar/pull/12818#discussion_r789340203.
Because when new PulsarAdmin sends the request to old broker, the old broker cannot recognize the JSON body like `{"partitions":3}`. /cc @codelipenghui
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787627405
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
Did you compile the latest code?
```bash
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3,"properties":{"k1":"v1","k2":"v2"}}% $ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X DELETE
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k3":"v3"}}'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3,"properties":{"k3":"v3"}}
```
If you used the original code, since the body must be an integer, the body `{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}` cannot be recognized.
```bash
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '3'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3}% $ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3"}'
Cannot deserialize value of type `int` from Object value (token `JsonToken.START_OBJECT`)
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 1]
```
But I don't think it breaks the backward compatibility. This PR makes the REST API available for more input.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787450205
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
I also tried `-d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'` and it works well.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787445274
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -215,7 +215,8 @@
prepareDynamicConfigurationMap();
private final ConcurrentOpenHashMap<String, Consumer<?>> configRegisteredListeners;
- private final ConcurrentLinkedQueue<Pair<String, CompletableFuture<Optional<Topic>>>> pendingTopicLoadingQueue;
+ private final ConcurrentLinkedQueue<Triple<String, CompletableFuture<Optional<Topic>>, Map<String, String>>>
Review comment:
Only PersistentTopic has the properties.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787415623
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -215,7 +215,8 @@
prepareDynamicConfigurationMap();
private final ConcurrentOpenHashMap<String, Consumer<?>> configRegisteredListeners;
- private final ConcurrentLinkedQueue<Pair<String, CompletableFuture<Optional<Topic>>>> pendingTopicLoadingQueue;
+ private final ConcurrentLinkedQueue<Triple<String, CompletableFuture<Optional<Topic>>, Map<String, String>>>
Review comment:
Could we add the properties map in class `Topic`?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787415623
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -215,7 +215,8 @@
prepareDynamicConfigurationMap();
private final ConcurrentOpenHashMap<String, Consumer<?>> configRegisteredListeners;
- private final ConcurrentLinkedQueue<Pair<String, CompletableFuture<Optional<Topic>>>> pendingTopicLoadingQueue;
+ private final ConcurrentLinkedQueue<Triple<String, CompletableFuture<Optional<Topic>>, Map<String, String>>>
Review comment:
Could we add the properties map in `Topic`?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787447327
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
@codelipenghui It looks like not. I ran the following command in my local env:
```bash
curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d 3
```
We can see the partitions number was recognized correctly.
--
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 commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r789340203
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
> If you used the original code, since the body must be an integer, the body {"partitions":"3","properties":{"k1":"v1","k2":"v2"}} cannot be recognized.
Yes, this is my test case, use the new Pulsar Admin version to point to the old broker.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787415623
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -215,7 +215,8 @@
prepareDynamicConfigurationMap();
private final ConcurrentOpenHashMap<String, Consumer<?>> configRegisteredListeners;
- private final ConcurrentLinkedQueue<Pair<String, CompletableFuture<Optional<Topic>>>> pendingTopicLoadingQueue;
+ private final ConcurrentLinkedQueue<Triple<String, CompletableFuture<Optional<Topic>>, Map<String, String>>>
Review comment:
Maybe we could add the properties map in class `Topic`?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r798665847
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/PersistentTopics.java
##########
@@ -0,0 +1,93 @@
+/**
+ * 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.v3;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.Encoded;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.container.AsyncResponse;
+import javax.ws.rs.container.Suspended;
+import javax.ws.rs.core.MediaType;
+import org.apache.pulsar.broker.admin.impl.PersistentTopicsBase;
+import org.apache.pulsar.common.partition.PartitionedTopicMetadata;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ */
+@Path("/persistent")
+@Produces(MediaType.APPLICATION_JSON)
+@Api(value = "/persistent", description = "Persistent topic admin apis", tags = "persistent topic")
+public class PersistentTopics extends PersistentTopicsBase {
Review comment:
my understanding in this patch was that we are using the v3 endpoint only in case of passing the "properties" and to use v2 in the other cases
it looks like it is kind of a simple but in the logic to choose the endpoint
--
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 commented on a change in pull request #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r786685136
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
##########
@@ -173,6 +173,34 @@ public void createPartitionedTopic(
}
}
+ @PUT
+ @Path("/{property}/{cluster}/{namespace}/{topic}/partitions/properties")
+ @ApiOperation(hidden = true, value = "Create a partitioned topic.",
Review comment:
I think we don't need to add new features in v1 version.
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r790706585
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -1239,6 +1251,13 @@ private WebTarget namespacePath(String domain, NamespaceName namespace, String..
return namespacePath;
}
+ private WebTarget topicPath(TopicName topic, Map<String, String> metadata, String... parts) {
+ final WebTarget base = metadata != null ? adminV3Topics : (topic.isV2() ? adminV2Topics : adminTopics);
Review comment:
Yes, when user create topics with properties, we will invoke v3.
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749876032
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java
##########
@@ -77,6 +77,7 @@
private int newEntriesCheckDelayInMillis = 10;
private Clock clock = Clock.systemUTC();
private ManagedLedgerInterceptor managedLedgerInterceptor;
+ private Map<String, String> topicMetadata;
Review comment:
Maybe we'd better don't add topic-related concepts in the ManagedLedger package, why not add this in the `PersistentTopic`?
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749886069
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java
##########
@@ -77,6 +77,7 @@
private int newEntriesCheckDelayInMillis = 10;
private Clock clock = Clock.systemUTC();
private ManagedLedgerInterceptor managedLedgerInterceptor;
+ private Map<String, String> topicMetadata;
Review comment:
@Jason918 @gaoran10
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749888166
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -368,9 +368,10 @@ public void revokePermissions(String topic, String role) throws PulsarAdminExcep
}
@Override
- public void createPartitionedTopic(String topic, int numPartitions) throws PulsarAdminException {
+ public void createPartitionedTopic(String topic, int numPartitions, Map<String, String> metadata)
+ throws PulsarAdminException {
try {
- createPartitionedTopicAsync(topic, numPartitions).get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
+ createPartitionedTopicAsync(topic, numPartitions, metadata).get(this.readTimeoutMs, TimeUnit.MILLISECONDS);
Review comment:
Ok, thanks for the suggestion.
--
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 #12818: PIP 110: Support Topic metadata
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r749946215
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java
##########
@@ -77,6 +77,7 @@
private int newEntriesCheckDelayInMillis = 10;
private Clock clock = Clock.systemUTC();
private ManagedLedgerInterceptor managedLedgerInterceptor;
+ private Map<String, String> topicMetadata;
Review comment:
Changed `topicMetadata` to `properties` which may cause confused
--
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 commented on a change in pull request #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787402534
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
Looks like a breaking change?
--
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 #12818: PIP 110: Support Topic metadata - PART-1 create topic with properties
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #12818:
URL: https://github.com/apache/pulsar/pull/12818#discussion_r787627405
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -225,17 +225,17 @@ public void createPartitionedTopic(
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic,
- @ApiParam(value = "The number of partitions for the topic",
- required = true, type = "int", defaultValue = "0")
- int numPartitions,
+ @ApiParam(value = "The metadata for the topic",
Review comment:
Did you compile the latest code? Here are the test input from my local env:
```bash
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3,"properties":{"k1":"v1","k2":"v2"}} $ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X DELETE
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3","properties":{"k3":"v3"}}'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3,"properties":{"k3":"v3"}}
```
If you used the original code, since the body must be an integer, the body `{"partitions":"3","properties":{"k1":"v1","k2":"v2"}}` cannot be recognized.
```bash
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '3'
$ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions
{"partitions":3}% $ curl -L http://localhost:8080/admin/v2/persistent/public/default/xxx/partitions -X PUT -H 'Content-Type: application/json' -d '{"partitions":"3"}'
Cannot deserialize value of type `int` from Object value (token `JsonToken.START_OBJECT`)
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 1]
```
But I don't think it breaks the backward compatibility. This PR makes the REST API available for more input.
--
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