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 2020/07/19 02:35:05 UTC
[GitHub] [pulsar] 315157973 opened a new pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
315157973 opened a new pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598
Fixes #7485
### Motivation
Support configuring DeleteInactiveTopic setting in namespace policy
### Modifications
Only the two parameters `brokerDeleteInactiveTopicsMode` and `brokerDeleteInactiveTopicsMaxInactiveDurationSeconds` support namespace policy. The parameters are changed to Map structure, the key is the namespace, and the value is the parameter value.
Such as: namespace1=delete_when_no_subscriptions, namespace2=delete_when_no_subscriptions.
In addition, there is a key name called `default`. If it is set, other namespaces that do not specify parameters will use this parameter.
Such as: default=delete_when_no_subscriptions
### Verifying this change
InactiveTopicDeleteTest.java
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#discussion_r458094494
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1696,6 +1746,9 @@ public CmdNamespaces(PulsarAdmin admin) {
jcommander.addCommand("set-delayed-delivery", new SetDelayedDelivery());
jcommander.addCommand("get-delayed-delivery", new GetDelayedDelivery());
+ jcommander.addCommand("get-inactive-topic", new GetInactiveTopicPolicies());
Review comment:
I changed it to `Policies`
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
##########
@@ -859,6 +860,30 @@ public void setDelayedDeliveryPolicies(@PathParam("tenant") String tenant,
internalSetDelayedDelivery(deliveryPolicies);
}
+ @GET
+ @Path("/{tenant}/{namespace}/inactiveTopic")
Review comment:
I changed it to `inactiveTopicPolicies`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#discussion_r458050955
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java
##########
@@ -98,8 +99,7 @@ public AbstractTopic(String topic, BrokerService brokerService) {
this.producers = new ConcurrentHashMap<>();
this.isFenced = false;
this.replicatorPrefix = brokerService.pulsar().getConfiguration().getReplicatorPrefix();
- this.deleteWhileInactive =
- brokerService.pulsar().getConfiguration().isBrokerDeleteInactiveTopicsEnabled();
+ this.inactiveTopicPolicies.setDeleteWhileInactive(brokerService.pulsar().getConfiguration().isBrokerDeleteInactiveTopicsEnabled());
Review comment:
If the namespace level policy already specified and then restart the broker, the inactive topic policy should apply the namespace level policy.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
##########
@@ -859,6 +860,30 @@ public void setDelayedDeliveryPolicies(@PathParam("tenant") String tenant,
internalSetDelayedDelivery(deliveryPolicies);
}
+ @GET
+ @Path("/{tenant}/{namespace}/inactiveTopic")
+ @ApiOperation(value = "Get inactive topic messages config on a namespace.")
+ @ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Tenant or cluster or namespace doesn't exist"),
+ @ApiResponse(code = 409, message = "Concurrent modification"), })
+ public InactiveTopicPolicies getInactiveTopicPolicies(@PathParam("tenant") String tenant,
+ @PathParam("namespace") String namespace) {
+ validateNamespaceName(tenant, namespace);
+ return internalGetInactiveTopic();
+ }
+
+ @POST
+ @Path("/{tenant}/{namespace}/inactiveTopic")
Review comment:
```suggestion
@Path("/{tenant}/{namespace}/inactiveTopicPolicy")
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1696,6 +1746,9 @@ public CmdNamespaces(PulsarAdmin admin) {
jcommander.addCommand("set-delayed-delivery", new SetDelayedDelivery());
jcommander.addCommand("get-delayed-delivery", new GetDelayedDelivery());
+ jcommander.addCommand("get-inactive-topic", new GetInactiveTopicPolicies());
Review comment:
```suggestion
jcommander.addCommand("get-inactive-topic-policy", new GetInactiveTopicPolicies());
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1696,6 +1746,9 @@ public CmdNamespaces(PulsarAdmin admin) {
jcommander.addCommand("set-delayed-delivery", new SetDelayedDelivery());
jcommander.addCommand("get-delayed-delivery", new GetDelayedDelivery());
+ jcommander.addCommand("get-inactive-topic", new GetInactiveTopicPolicies());
+ jcommander.addCommand("set-inactive-topic", new SetInactiveTopicPolicies());
Review comment:
```suggestion
jcommander.addCommand("set-inactive-topic-policy", new SetInactiveTopicPolicies());
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1014,6 +1016,54 @@ void run() throws PulsarAdminException {
}
}
+ @Parameters(commandDescription = "Get the inactive topic policy for a namespace")
+ private class GetInactiveTopicPolicies extends CliCommand {
+ @Parameter(description = "tenant/namespace\n", required = true)
+ private java.util.List<String> params;
+
+ @Override
+ void run() throws PulsarAdminException {
+ String namespace = validateNamespace(params);
+ print(admin.namespaces().getInactiveTopicPolicies(namespace));
+ }
+ }
+
+ @Parameters(commandDescription = "Set the inactive topic policies on a namespace")
+ private class SetInactiveTopicPolicies extends CliCommand {
+ @Parameter(description = "tenant/namespace", required = true)
+ private java.util.List<String> params;
+
+ @Parameter(names = { "--enable", "-e" }, description = "Enable inactive topic messages")
+ private boolean enable = false;
+
+ @Parameter(names = { "--disable", "-d" }, description = "Disable inactive topic messages")
+ private boolean disable = false;
+
+ @Parameter(names = {"--max-inactive-duration", "-t"}, description = "Max duration of topic inactivity in seconds" +
+ ",topics that are inactive for longer than this value will be deleted (eg: 1s, 10s, 1m, 5h, 3d)", required = true)
+ private String deleteInactiveTopicsMaxInactiveDuration;
+
+ @Parameter(names = { "--delete-mode", "-m" }, description = "Disable inactive topic messages", required = true)
+ private String inactiveTopicDeleteMode;
Review comment:
It's better to provide the options of `inactiveTopicDeleteMode`. So that users can know what to fill in.
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java
##########
@@ -1014,6 +1016,54 @@ void run() throws PulsarAdminException {
}
}
+ @Parameters(commandDescription = "Get the inactive topic policy for a namespace")
+ private class GetInactiveTopicPolicies extends CliCommand {
+ @Parameter(description = "tenant/namespace\n", required = true)
+ private java.util.List<String> params;
+
+ @Override
+ void run() throws PulsarAdminException {
+ String namespace = validateNamespace(params);
+ print(admin.namespaces().getInactiveTopicPolicies(namespace));
+ }
+ }
+
+ @Parameters(commandDescription = "Set the inactive topic policies on a namespace")
+ private class SetInactiveTopicPolicies extends CliCommand {
+ @Parameter(description = "tenant/namespace", required = true)
+ private java.util.List<String> params;
+
+ @Parameter(names = { "--enable", "-e" }, description = "Enable inactive topic messages")
+ private boolean enable = false;
+
+ @Parameter(names = { "--disable", "-d" }, description = "Disable inactive topic messages")
+ private boolean disable = false;
Review comment:
use `deleteWhileInactive` here is more reasonable because of the command name is `set-inactive-topic`.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
##########
@@ -859,6 +860,30 @@ public void setDelayedDeliveryPolicies(@PathParam("tenant") String tenant,
internalSetDelayedDelivery(deliveryPolicies);
}
+ @GET
+ @Path("/{tenant}/{namespace}/inactiveTopic")
Review comment:
```suggestion
@Path("/{tenant}/{namespace}/inactiveTopicPolicy")
```
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/InactiveTopicPolicies.java
##########
@@ -0,0 +1,35 @@
+/**
+ * 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.common.policies.data;
+
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/**
+ * Definition of the inactive topic policy.
+ */
+@Data
+@AllArgsConstructor
+@NoArgsConstructor
+public class InactiveTopicPolicies {
+ private InactiveTopicDeleteMode inactiveTopicDeleteMode;
+ private int brokerDeleteInactiveTopicsMaxInactiveDurationSeconds;
Review comment:
```suggestion
private int maxInactiveDurationSeconds;
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java
##########
@@ -829,11 +829,14 @@ public boolean isActive() {
}
@Override
- public void checkGC(int maxInactiveDurationInSec, InactiveTopicDeleteMode deleteMode) {
- if (!deleteWhileInactive) {
+ public void checkGC(int maxInactiveDurationSeconds, InactiveTopicDeleteMode inactiveTopicDeleteMode) {
+ if (!isDeleteWhileInactive()) {
Review comment:
If we already use InactiveTopicPolicies, I think we don't need to pass `maxInactiveDurationSeconds` and `inactiveTopicDeleteMode` in the param list. Of course, if only use InactiveTopicPolicies to maintain the namespace level policy, we still need to pass parameters.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -1628,11 +1628,17 @@ private boolean hasBacklogs() {
}
@Override
- public void checkGC(int maxInactiveDurationInSec, InactiveTopicDeleteMode deleteMode) {
- if (!deleteWhileInactive) {
+ public void checkGC(int maxInactiveDurationSeconds, InactiveTopicDeleteMode inactiveTopicDeleteMode) {
+ if (!isDeleteWhileInactive()) {
Review comment:
Same as the comment that I left in the NonPersistentTopic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#discussion_r458476303
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
##########
@@ -859,6 +860,30 @@ public void setDelayedDeliveryPolicies(@PathParam("tenant") String tenant,
internalSetDelayedDelivery(deliveryPolicies);
}
+ @GET
+ @Path("/{tenant}/{namespace}/inactiveTopic")
Review comment:
Sounds good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wolfstudy commented on pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#issuecomment-664806421
> I think this should be part of the same structure that is already used for topic auto-creation, since auto-creation and auto-deletion are tightly related
Sorry @merlimat I merged into this pull request by mistake. If necessary, can we revert this change or open a new issue to fix the corresponding problem?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#issuecomment-664769657
@merlimat Do you mean we should change `InactiveTopicPolicy` to `TopicAutoDeletionPolicy`? And change the command in the namespaces to `set-topic-auto-creation`, `get-topic-auto-creation`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#discussion_r458093542
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java
##########
@@ -98,8 +99,7 @@ public AbstractTopic(String topic, BrokerService brokerService) {
this.producers = new ConcurrentHashMap<>();
this.isFenced = false;
this.replicatorPrefix = brokerService.pulsar().getConfiguration().getReplicatorPrefix();
- this.deleteWhileInactive =
- brokerService.pulsar().getConfiguration().isBrokerDeleteInactiveTopicsEnabled();
+ this.inactiveTopicPolicies.setDeleteWhileInactive(brokerService.pulsar().getConfiguration().isBrokerDeleteInactiveTopicsEnabled());
Review comment:
I have fixed it. If there is a namespace level policy, the policy will be restored from zk in the subclass construction 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wolfstudy merged pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on a change in pull request #7598: Support configuring DeleteInactiveTopic setting in namespace policy
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #7598:
URL: https://github.com/apache/pulsar/pull/7598#discussion_r457020544
##########
File path: pulsar-broker/src/test/resources/configurations/pulsar_broker_test.conf
##########
@@ -85,7 +85,7 @@ replicationMetricsEnabled=true
replicationConnectionsPerBroker=16
replicationProducerQueueSize=1000
replicatorPrefix=pulsar.repl
-brokerDeleteInactiveTopicsMode=delete_when_subscriptions_caught_up
+brokerDeleteInactiveTopicsMode=default=delete_when_subscriptions_caught_up
Review comment:
Is this change needed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org