Support get topic applied policy for message TTL #9225

315157973 · 2021-01-17T07:43:41Z

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

315157973 · 2021-01-17T10:42:18Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java

zymap

Overall looks good. Left some suggested changes.

pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java

pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java

pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/TopicsImpl.java

…in/internal/TopicsImpl.java Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>

zymap · 2021-01-19T01:30:51Z

/pulsarbot run-failure-checks

315157973 · 2021-01-19T05:57:59Z

/pulsarbot run-failure-checks

eolivelli

Good work.
I have left a comment about backward compatibility, in my opinion we cannot change the behaviour of the existing method.

Also I would rename 'applied' to 'actual'

eolivelli · 2021-01-19T07:37:41Z

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;


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

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

@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.

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.

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.

It is different from breaking a change that is introduced in old versions than 2.7.x.

I see.
let's fix it for 2.7.x

sijie · 2021-01-19T10:28:06Z

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".

eolivelli · 2021-01-19T10:31:49Z

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

315157973 · 2021-01-19T10:50:54Z

/pulsarbot run-failure-checks

sijie · 2021-01-19T10:52:26Z

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.

Jennifer88huang-zz · 2021-01-19T11:40:37Z

@315157973 what's the impact on docs? or what's the impact on end users?

315157973 · 2021-01-19T11:47:03Z

@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

315157973 · 2021-01-19T12:23:06Z

/pulsarbot run-failure-checks

Jennifer88huang-zz · 2021-01-19T13:42:22Z

@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?

315157973 · 2021-01-19T13:56:05Z

@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

Jennifer88huang-zz · 2021-01-19T14:00:14Z

@315157973 Got it, thank you very much for your detailed explanation.
We can add the related doc content when you unify them.

315157973 · 2021-01-20T03:02:49Z

/pulsarbot run-failure-checks

315157973 · 2021-01-20T03:40:36Z

/pulsarbot run-failure-checks

315157973 · 2021-01-20T03:56:54Z

/pulsarbot run-failure-checks

Anonymitaet · 2021-03-08T09:35:36Z

@315157973 thanks for your great work! Shall the changes be added to the Admin API - Topics section? If so, could you please help add docs accordingly? Thanks

315157973 · 2021-03-08T09:41:45Z

@315157973 thanks for your great work! Shall the changes be added to the Admin API - Topics section? If so, could you please help add docs accordingly? Thanks

When #9216 is over, I will add all the documents.

Anonymitaet · 2021-03-08T09:46:22Z

@315157973 thanks! You can ping me if you need a doc review.

Anonymitaet · 2021-04-15T05:13:45Z

Docs have been added to source code files.

Master Issue: apache#9216 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. 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 (cherry picked from commit 54b083b)

Support get topic applied policy for message TTL

e434c68

zymap reviewed Jan 17, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java Outdated Show resolved Hide resolved

codelipenghui assigned 315157973 Jan 17, 2021

codelipenghui added this to the 2.8.0 milestone Jan 17, 2021

sijie added the component/topic-policy label Jan 18, 2021

315157973 added 2 commits January 18, 2021 15:40

remove api

b6fe7d0

code style

927bf2a

zymap reviewed Jan 18, 2021

View reviewed changes

315157973 and others added 2 commits January 18, 2021 22:48

Update pulsar-client-admin/src/main/java/org/apache/pulsar/client/adm…

56084ad

…in/internal/TopicsImpl.java Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>

change api name

ecdd7cc

zymap approved these changes Jan 19, 2021

View reviewed changes

eolivelli requested changes Jan 19, 2021

View reviewed changes

Jennifer88huang-zz added the doc-required Your PR changes impact docs and you will update later. label Jan 19, 2021

codelipenghui approved these changes Jan 20, 2021

View reviewed changes

codelipenghui merged commit 54b083b into apache:master Jan 20, 2021

315157973 deleted the MessageTTL branch February 23, 2021 13:01

Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support get topic applied policy for message TTL #9225

Support get topic applied policy for message TTL #9225

315157973 commented Jan 17, 2021

315157973 commented Jan 17, 2021

zymap left a comment

zymap commented Jan 19, 2021

315157973 commented Jan 19, 2021

eolivelli left a comment

eolivelli Jan 19, 2021

315157973 Jan 19, 2021

codelipenghui Jan 19, 2021

eolivelli Jan 19, 2021

sijie Jan 19, 2021

sijie Jan 19, 2021

eolivelli Jan 19, 2021

sijie commented Jan 19, 2021

eolivelli commented Jan 19, 2021

315157973 commented Jan 19, 2021

sijie commented Jan 19, 2021

Jennifer88huang-zz commented Jan 19, 2021 •

edited

Loading

315157973 commented Jan 19, 2021

315157973 commented Jan 19, 2021

Jennifer88huang-zz commented Jan 19, 2021

315157973 commented Jan 19, 2021

Jennifer88huang-zz commented Jan 19, 2021

315157973 commented Jan 20, 2021

315157973 commented Jan 20, 2021

315157973 commented Jan 20, 2021

Anonymitaet commented Mar 8, 2021

315157973 commented Mar 8, 2021

Anonymitaet commented Mar 8, 2021

Anonymitaet commented Apr 15, 2021

Support get topic applied policy for message TTL #9225

Support get topic applied policy for message TTL #9225

Conversation

315157973 commented Jan 17, 2021

Motivation

Modifications

Verifying this change

315157973 commented Jan 17, 2021

zymap left a comment

Choose a reason for hiding this comment

zymap commented Jan 19, 2021

315157973 commented Jan 19, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Jan 19, 2021

Choose a reason for hiding this comment

315157973 Jan 19, 2021

Choose a reason for hiding this comment

codelipenghui Jan 19, 2021

Choose a reason for hiding this comment

eolivelli Jan 19, 2021

Choose a reason for hiding this comment

sijie Jan 19, 2021

Choose a reason for hiding this comment

sijie Jan 19, 2021

Choose a reason for hiding this comment

eolivelli Jan 19, 2021

Choose a reason for hiding this comment

sijie commented Jan 19, 2021

eolivelli commented Jan 19, 2021

315157973 commented Jan 19, 2021

sijie commented Jan 19, 2021

Jennifer88huang-zz commented Jan 19, 2021 • edited Loading

315157973 commented Jan 19, 2021

315157973 commented Jan 19, 2021

Jennifer88huang-zz commented Jan 19, 2021

315157973 commented Jan 19, 2021

Jennifer88huang-zz commented Jan 19, 2021

315157973 commented Jan 20, 2021

315157973 commented Jan 20, 2021

315157973 commented Jan 20, 2021

Anonymitaet commented Mar 8, 2021

315157973 commented Mar 8, 2021

Anonymitaet commented Mar 8, 2021

Anonymitaet commented Apr 15, 2021

Jennifer88huang-zz commented Jan 19, 2021 •

edited

Loading