Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the time unit of patternAutoDiscoveryPeriod to SECONDS #5927

Closed
sijie opened this issue Dec 24, 2019 · 2 comments · Fixed by #5950
Closed

Change the time unit of patternAutoDiscoveryPeriod to SECONDS #5927

sijie opened this issue Dec 24, 2019 · 2 comments · Fixed by #5950
Labels
area/client help wanted type/feature The PR added a new feature or issue requested a new feature

Comments

@sijie
Copy link
Member

sijie commented Dec 24, 2019

Is your feature request related to a problem? Please describe.

Currently the time unit of patternAutoDiscoveryPeriod is MINUTE. People is not able to configure the time duration to be leass than 1 minute.

Describe the solution you'd like

It would be good to add an overloaded method in the consumer builder to support passing TimeUnit in.

patternAutoDiscoveryPeriod(int interval, TimeUnit unit);
@sijie sijie added help wanted area/client type/feature The PR added a new feature or issue requested a new feature labels Dec 24, 2019
@zhenglaizhang
Copy link
Contributor

i think i could help add the overload this week.

@sijie
Copy link
Member Author

sijie commented Dec 24, 2019

@zhenglaizhang awesome!

sijie pushed a commit that referenced this issue Feb 8, 2020
…veryPeriod` to SECONDS (#5950)

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)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this issue Aug 24, 2020
…veryPeriod` to SECONDS (apache#5950)

Fixes apache#5927 

### Motivation
Per apache#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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client help wanted type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
3 participants