You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/08 18:27:54 UTC
[GitHub] [pulsar] ciaocloud opened a new pull request #13195: Support of TopicLifecyclePolicies REST handler.
ciaocloud opened a new pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
Fixes #<xyz>
*(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)*
Master Issue: #<xyz>
-->
Originally authored by @merlimat, fixed for compatibility with 2.9 branch.
### Motivation
*Support of TopicLifecyclePolicies REST handler.*
### Modifications
*Describe the modifications you've done.*
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as *(please describe tests)*.
### 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): ( no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [ ] `doc-required`
(If you need help on updating docs, create a doc issue)
- [ ] `no-need-doc`
(Please explain why)
- [ ] `doc`
(If this PR contains doc changes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#issuecomment-989543679
@ciaocloud:Thanks for providing doc info!
--
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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765490500
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
Review comment:
ack. 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] github-actions[bot] commented on pull request #13195: Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#issuecomment-989074096
@ciaocloud: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] github-actions[bot] commented on pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#issuecomment-1051439651
The pr had no activity for 30 days, mark with Stale label.
--
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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765461560
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
+ // reset new inactiveTopicPolicies with topicLifecycle info if one exists
+ this.topicPolicies.getInactiveTopicPolicies().updateBrokerValue(
Review comment:
we wished to set the `inactiveTopicPolicies` as the new one, isn't it the right way to do it with your recent changes? in 2.9, the `inactiveTopicPolicies` was a member field in 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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765466311
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
+ // reset new inactiveTopicPolicies with topicLifecycle info if one exists
+ this.topicPolicies.getInactiveTopicPolicies().updateBrokerValue(
Review comment:
o.k., we'll need to use `updateNamespaceValue` to reset the `inactiveTopicPolicies`, 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] Anonymitaet commented on pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#issuecomment-989447528
Hi @ciaocloud is this a code cleanup work and do not need to update docs?
when submitting a PR, can you help provide a doc label (tick the box) in the [PR template which contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation)? This helps others know more about the changes. Thanks
<img width="682" alt="Documentation" src="https://user-images.githubusercontent.com/50226895/145323454-0008a85e-3c80-49da-baba-6d41b46e753e.png">
--
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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765490500
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
Review comment:
ack. 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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765461560
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
+ // reset new inactiveTopicPolicies with topicLifecycle info if one exists
+ this.topicPolicies.getInactiveTopicPolicies().updateBrokerValue(
Review comment:
we wished to set the `inactiveTopicPolicies` as the new one, isn't it the right way to do it with your recent changes? in 2.9 and earlier versions, the `inactiveTopicPolicies` was a member field in 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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765491393
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
+ // reset new inactiveTopicPolicies with topicLifecycle info if one exists
+ this.topicPolicies.getInactiveTopicPolicies().updateBrokerValue(
Review comment:
it's about topic creation/deletion, so we should `updateNamespaceValue`, 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] Jason918 commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765389268
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
Review comment:
We should update all topicPolicies values in `updateTopicPolicyByNamespacePolicy`
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/TopicLifecyclePolicies.java
##########
@@ -0,0 +1,44 @@
+/**
+ * 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;
+
+/**
+ * Topic life-cycle policies.
+ */
+@Data
+@AllArgsConstructor
+@NoArgsConstructor
+public class TopicLifecyclePolicies {
+
+ /**
+ * Whether topics should be automatically created when producers/consumers connect, or rather explicitly created
+ * through admin API.
+ */
+ private boolean autoCreateTopics;
+
+ /**
+ * Whether topics should be automatically deleted when inactive, or rather or rather explicitly deleted through
Review comment:
Typo "or rather or rather"
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
+ // reset new inactiveTopicPolicies with topicLifecycle info if one exists
+ this.topicPolicies.getInactiveTopicPolicies().updateBrokerValue(
Review comment:
Why do we update `brokerValue` in namespace policy?
--
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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765487797
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/TopicLifecyclePolicies.java
##########
@@ -0,0 +1,44 @@
+/**
+ * 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;
+
+/**
+ * Topic life-cycle policies.
+ */
+@Data
+@AllArgsConstructor
+@NoArgsConstructor
+public class TopicLifecyclePolicies {
+
+ /**
+ * Whether topics should be automatically created when producers/consumers connect, or rather explicitly created
+ * through admin API.
+ */
+ private boolean autoCreateTopics;
+
+ /**
+ * Whether topics should be automatically deleted when inactive, or rather or rather explicitly deleted through
Review comment:
> I am confused about the relation between `InactiveTopicPolicies` and `TopicLifecycle`. Can you bring more details?
Both `InactiveTopicPolicies` and `TopicLifecycle` are fields declared in `Policies`.
The `InactiveTopicPolicies` has a boolean value `deleteWhileInactive` which determines whether topics should be automatically deleted when inactive.
We ask it to be same as that defined in `TopicLifecycle`, while `TopicLifecycle` also defines whether topics should be automatically created.
This endpoint is used during tenant creation time.
(Also would update the above in 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] ciaocloud commented on a change in pull request #13195: [pulsar-broker] Support of TopicLifecyclePolicies REST handler.
Posted by GitBox <gi...@apache.org>.
ciaocloud commented on a change in pull request #13195:
URL: https://github.com/apache/pulsar/pull/13195#discussion_r765519388
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2485,6 +2493,13 @@ private boolean shouldTopicBeRetained() {
//If the topic-level policy already exists, the namespace-level policy cannot override the topic-level policy.
Optional<TopicPolicies> topicPolicies = getTopicPolicies();
+ if (data.topicLifecycle != null) {
Review comment:
We can definitely refactor it into the `updateTopicPolicyByNamespacePolicy` method in `AbstractTopic`, but as that method would be used by both `PersistentTopic` and `NonPersistentTopic`, not sure it is the right thing to do for the non-persistent one to update `inactiveTopicPolices` with `topicLifecycle` info. will need to double-check.
--
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