[Issue 5927][Pulsar Client] Change the time unit of `patternAutoDiscoveryPeriod to seconds

[zhenglaizhang]

Fixes #5927

Motivation

Per #5927, by adding one overload to configure patternAutoDiscoveryPeriod by time units, users may be able to configure the time duration to be less than 1 minute.

Modifications

1. Add an overload of ConsumerBuilder.patternAutoDiscoveryPeriod
2. Update original ConsumerBuilderImpl.patternAutoDiscoveryPeriod to call the new overload
3. Update the PatternMultiTopicsConsumerImpl.recheckPatternTimeout logic, this should be carefully reviewed, since I don't understand the original logic, it seems the old implementation will only allow 0 or 1 minute as the recheck interval, ignoring the user's periods which longer than 1 minute

Verifying this change

• Add the unit test to verify param invalidness
• Update one unit test in PatternTopicsConsumerImplTest.testAutoUnbubscribePatternConsumer to use 10 seconds period
  (if another new test is requested per review i will add it later)

Does this pull request potentially affect one of the following parts:

• Dependencies (does it add or upgrade a dependency): (no)
• The public API: (yes) ( I think the consumer builder might be classified as public api, plz help correct me if wrong, thx)
• 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

• Does this pull request introduce a new feature? (yes)
• If yes, how is the feature documented? (JavaDocs)

Future Discussion

• Personally I think the new overload patternAutoDiscoveryPeriod(interval,unit) may have more meaningful semantic, will there be any plan to deprecate the old method?

[jiazhai]

retest this please

[codelipenghui]

retest this please

[sijie]

run java8 tests
run integration tests

[merlimat]

The reason for limit the min refresh to 1 min is to avoid clients creating unnecessary load to brokers with a custom configuration. If you set 10ms, it could create a lot of requests and significant load if the number of topics is not trivial.

Of course, the proper solution would be to have the broker notifying clients whenever there are new topics.

[zhenglaizhang]

The reason for limit the min refresh to 1 min is to avoid clients creating unnecessary load to brokers with a custom configuration. If you set 10ms, it could create a lot of requests and significant load if the number of topics is not trivial.

Of course, the proper solution would be to have the broker notifying clients whenever there are new topics.

understood, my update will ensure the min interval is 1 second, which might still be too short.

agree with you, [the broker notifying clients] will be the final great solution.

[sijie]

I think this change doesn’t change the default value. This provides the mechanism for applications to configure this in a finer granularity as people want reduce this to a few seconds.

[sijie]

run java8 tests

[sijie]

run java8 tests

[sijie]

run java8 tests

[EugenDueck]

+1 I was just about to submit my own PR that changes Math.min(1, conf.getPatternAutoDiscoveryPeriod()) to Math.max(1, conf.getPatternAutoDiscoveryPeriod()) - when I noticed this PR and realized that it contains this fix.

Of course, the proper solution would be to have the broker notifying clients whenever there are new topics.

I agree, and if this is actually realistic (I have not seen enough of the pulsar code base yet to make that call) I would even volunteer to implement this, because it is something that I would like to have, as the use case I have started to play with has tens of thousands of topics in a single namespace, i.e. all these topics are subject to multi-topic pattern subscription. And for that, a push notification for topic additions and removals would seem much more lightweight than having to pull, say, 20 thousand topic names every second or so, even when there aren't (m)any changes.