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/01/17 07:43:52 UTC
[GitHub] [pulsar] 315157973 opened a new pull request #9225: Support get topic applied policy for message TTL
315157973 opened a new pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225
Master Issue: #9216
### Motivation
Provide a way to get the applied policy of the topic. In Pulsar, the topic will apply the topic level policy if present or apply the namespace policy if present or use the broker level configuration. It needs to support get the applied policy for a topic that might use the namespace policy or topic policy or broker level default configuration.
### Modifications
1 Now if there is no data at the namespace-level, the broker-level data will be returned by default, so I fix it
2 Now if there is no data at the topic-level, the data at the namespace-level will be returned by default, so I fix it
3 Add applied interface
4 Since the initial value becomes null, the corresponding unit test is modified
### Verifying this change
unit test:
testDifferentLevelPolicyApplied
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762619877
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559182821
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1521,20 +1521,36 @@ public void removeBacklogQuota(@Suspended final AsyncResponse asyncResponse,
internalRemoveBacklogQuota(asyncResponse, backlogQuotaType);
}
+ @GET
+ @Path("/{tenant}/{namespace}/{topic}/messageTTLApplied")
Review comment:
+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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762808046
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762960975
----------------------------------------------------------------
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] zymap commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559554934
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -2011,6 +2011,18 @@ public int getMessageTTL(String topic) throws PulsarAdminException {
}
}
+ @Override
+ public int getMessageTTLApplied(String topic) throws PulsarAdminException {
Review comment:
```suggestion
Integer getMessageTTL(String topic, boolean applied) throws PulsarAdminException {
```
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
+
+ /**
+ * Get message TTL applied for a topic.
+ * @param topic
+ * @return
+ * @throws PulsarAdminException
+ */
+ int getMessageTTLApplied(String topic) throws PulsarAdminException;
Review comment:
```suggestion
Integer getMessageTTL(String topic, boolean applied) throws PulsarAdminException;
```
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -2001,7 +2001,7 @@ public void setMessageTTL(String topic, int messageTTLInSecond) throws PulsarAdm
}
@Override
- public int getMessageTTL(String topic) throws PulsarAdminException {
+ public Integer getMessageTTL(String topic) throws PulsarAdminException {
Review comment:
```suggestion
public Integer getMessageTTL(String topic) throws PulsarAdminException {
return getMessageTTL(topic, false);
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -1081,10 +1081,17 @@ void run() throws PulsarAdminException {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-ap", "--applied" }, description = "Get the applied policy of the topic")
+ private boolean applied = false;
+
@Override
void run() throws PulsarAdminException {
String persistentTopic = validatePersistentTopic(params);
- print(admin.topics().getMessageTTL(persistentTopic));
+ if (applied) {
+ print(admin.topics().getMessageTTLApplied(persistentTopic));
+ } else {
+ print(admin.topics().getMessageTTL(persistentTopic));
+ }
Review comment:
```suggestion
print(admin.topics().getMessageTTL(persistentTopic, applied));
```
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java
##########
@@ -2011,6 +2011,18 @@ public int getMessageTTL(String topic) throws PulsarAdminException {
}
}
+ @Override
+ public int getMessageTTLApplied(String topic) throws PulsarAdminException {
+ try {
+ TopicName topicName = validateTopic(topic);
+ WebTarget path = topicPath(topicName, "messageTTL");
+ path = path.queryParam("applied", true);
Review comment:
```suggestion
path = path.queryParam("applied", applied);
```
----------------------------------------------------------------
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] sijie commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762763971
> what about 'mergedWithDefaults', or 'defaultsApplied' ?
"withDefaults" doesn't work in this case. Because there are multiple layers, it merged namespace-level and broker-level configs together. Hence "merged" or "applied" works there.
----------------------------------------------------------------
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] Anonymitaet commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-820101289
Docs have been added to source code files.
--
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763294411
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763284827
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762791157
> @315157973 what's the impact on docs? or what's the impact on end users?
Need to indicate the meaning of the return value
value == null: The default value, no parameters are set at this level
value == 0: This policy is disabled at this level
value> 0: Enable according to the set value
----------------------------------------------------------------
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] zymap commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559262652
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1521,20 +1521,36 @@ public void removeBacklogQuota(@Suspended final AsyncResponse asyncResponse,
internalRemoveBacklogQuota(asyncResponse, backlogQuotaType);
}
+ @GET
+ @Path("/{tenant}/{namespace}/{topic}/messageTTLApplied")
Review comment:
I mean like this:
```
/{tenant}/{namespace}/{topic}/inactiveTopicPolicies?applied=true
```
By default, it can be false
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r560070409
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
topic policy is a newer feature that is not advertised as a stable change yet in 2.7.0. I think we should fix the bad behavior in the 2.7.x release.
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762965471
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762612187
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762752950
> > Also I would rename 'applied' to 'actual'
>
> "actual" is a confusing word. What does "actual" mean? Is it an "actual" value of the topic or a different thing?
>
> "applied" or "merged" is better than "actual".
what about 'mergedWithDefaults', or 'defaultsApplied' ?
not a big deal for me, I was just suggesting a word that was more meaningful for me
----------------------------------------------------------------
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 #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559183498
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1521,20 +1521,36 @@ public void removeBacklogQuota(@Suspended final AsyncResponse asyncResponse,
internalRemoveBacklogQuota(asyncResponse, backlogQuotaType);
}
+ @GET
+ @Path("/{tenant}/{namespace}/{topic}/messageTTLApplied")
Review comment:
If we reuse the original GET method, compatibility is a problem. I have tried the following paths, but none of them are compatible:
```
/{tenant}/{namespace}/{topic}/{applied}/inactiveTopicPolicies
/{tenant}/{namespace}/{topic}/inactiveTopicPolicies/{applied}
```
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762960975
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r560057909
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
@eolivelli This method is introduced in 2.7.0(topic level policy) and I think this is a mistake in 2.7.0, most of the `get policy` method are returned Integer or Long, and the old behavior is the wrong behavior. I think we can clarify it in the release doc, I prefer to fix it directly. If left this method for future releases, users might be confused more.
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-792625216
> @315157973 thanks for your great work! Shall the changes be added to the [Admin API - Topics](http://pulsar.incubator.apache.org/docs/en/next/admin-api-topics/) section? If so, could you please help add docs accordingly? Thanks
When #9216 is over, I will add all the documents.
----------------------------------------------------------------
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 #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559984779
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
Methods with the same parameters and names cannot coexist. This is a problem left over from history. If we want to switch completely smoothly, we can add interfaces like admin.topicsV2(), admin.namespaceV2()...
If we modify the interface directly, users will know when upgrading the SDK(It will definitely trigger a recompilation when upgrading the SDK), we need to improve the doc of each interface, clarify the meaning of each return value
Please take a look here @sijie @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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-761769330
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559302014
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1521,20 +1521,36 @@ public void removeBacklogQuota(@Suspended final AsyncResponse asyncResponse,
internalRemoveBacklogQuota(asyncResponse, backlogQuotaType);
}
+ @GET
+ @Path("/{tenant}/{namespace}/{topic}/messageTTLApplied")
Review comment:
This way is different from the previous, but I think it is OK. what do you think? @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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762942966
----------------------------------------------------------------
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] Jennifer88huang commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762846482
@315157973 Thank you for your info.
Just confirm, in #9216, we have 14 commands, does the impact apply to all those cases? Is the impact same for all of them?
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763296061
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762609261
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763312486
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762856516
@315157973 Got it, thank you very much for your detailed explanation.
We can add the related doc content when you unify them.
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762908926
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559564896
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
+
+ /**
+ * Get message TTL applied for a topic.
+ * @param topic
+ * @return
+ * @throws PulsarAdminException
+ */
+ int getMessageTTLApplied(String topic) throws PulsarAdminException;
Review comment:
How about we set `int getMessageTTLApplied(String topic)` to `@Deprecated`, and add a new `Integer getMessageTTL(String topic, boolean applied)`?
----------------------------------------------------------------
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] eolivelli commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559966114
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
this is a breaking change in the API,
code compiled for previous versions of Pulsar client will have to be re-compiled.
And also this is a behaviour change, recompiling is not enough to apply the new meaning of the value.
People who simply recompile will see NPE
I would not change this method and make it that returns the same value as before.
we are adding the new method and that will be the right way.
Here we have to add a javadoc that explains the meaning of the result,
we could also deprecate this method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r560065180
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
We must pay more attention to public APIs, this kind of changes will hurt users, probably someone will get an unexpected NPE
I would accept to change the signature for a 2.8. release, but please do not cherry pick this change to 2.7 branch.
----------------------------------------------------------------
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] eolivelli commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r560073405
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
I see.
let's fix it for 2.7.x
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762958805
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762770079
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763284827
----------------------------------------------------------------
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] sijie commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559318463
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1521,20 +1521,36 @@ public void removeBacklogQuota(@Suspended final AsyncResponse asyncResponse,
internalRemoveBacklogQuota(asyncResponse, backlogQuotaType);
}
+ @GET
+ @Path("/{tenant}/{namespace}/{topic}/messageTTLApplied")
Review comment:
We should use the query parameter instead of a path parameter.
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762763111
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762544877
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-792621430
@315157973 thanks for your great work! Shall the changes be added to the [Admin API - Topics](http://pulsar.incubator.apache.org/docs/en/next/admin-api-topics/) section? If so, could you please help add docs accordingly? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762967436
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762770079
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762854379
> @315157973 Thank you for your info.
>
> Just confirm, in #9216, we have 14 commands, does the impact apply to all those cases? Is the impact same for all of them?
All will be unified soon.
For get method：
If applied=true, the topic will return the topic level policy if present or return the namespace policy if present or return the broker level configuration.
If applied=false, only return current level policy.the rules will be the same as the following:
value == null: The default value, no parameters are set at this level
value == 0: This policy is disabled at this level
value> 0: Enable according to the set value
----------------------------------------------------------------
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 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762967436
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762908926
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763283740
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762972626
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762788217
@315157973 what's the impact on docs?
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762963668
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-763307908
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-792627956
@315157973 thanks! You can ping me if you need a doc review.
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762942966
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang edited a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
Jennifer88huang edited a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762788217
@315157973 what's the impact on docs? or what's the impact on end users?
----------------------------------------------------------------
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 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762612187
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 removed a comment on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762972626
----------------------------------------------------------------
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 #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559564896
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
+
+ /**
+ * Get message TTL applied for a topic.
+ * @param topic
+ * @return
+ * @throws PulsarAdminException
+ */
+ int getMessageTTLApplied(String topic) throws PulsarAdminException;
Review comment:
How about we set `int getMessageTTLApplied(String topic)` to `@Deprecated`, and add a new `Integer getMessageTTL(String topic, boolean applied)`?
----------------------------------------------------------------
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] zymap commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559178470
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1521,20 +1521,36 @@ public void removeBacklogQuota(@Suspended final AsyncResponse asyncResponse,
internalRemoveBacklogQuota(asyncResponse, backlogQuotaType);
}
+ @GET
+ @Path("/{tenant}/{namespace}/{topic}/messageTTLApplied")
Review comment:
I am curious about why not add a query Params in the original API rather introduce a new API?
----------------------------------------------------------------
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 pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762609261
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r560070681
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
Review comment:
It is different from breaking a change that is introduced in old versions than 2.7.x.
----------------------------------------------------------------
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 #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#discussion_r559564896
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1604,7 +1604,15 @@ void setDelayedDeliveryPolicy(String topic
* @throws PulsarAdminException
* Unexpected error
*/
- int getMessageTTL(String topic) throws PulsarAdminException;
+ Integer getMessageTTL(String topic) throws PulsarAdminException;
+
+ /**
+ * Get message TTL applied for a topic.
+ * @param topic
+ * @return
+ * @throws PulsarAdminException
+ */
+ int getMessageTTLApplied(String topic) throws PulsarAdminException;
Review comment:
How about we set `int getMessageTTLApplied(String topic)` to @Deprecated, and add a new `Integer getMessageTTL(String topic, boolean applied)`?
----------------------------------------------------------------
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] sijie commented on pull request #9225: Support get topic applied policy for message TTL
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9225:
URL: https://github.com/apache/pulsar/pull/9225#issuecomment-762751069
> Also I would rename 'applied' to 'actual'
"actual" is a confusing word. What does "actual" mean? Is it an "actual" value of the topic or a different thing?
"applied" or "merged" is better than "actual".
----------------------------------------------------------------
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