-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[PIP-51] Introduce sticky consumer #5388
Conversation
run java8 tests |
1 similar comment
run java8 tests |
run java8 tests |
retest this please |
2 similar comments
retest this please |
retest this please |
rerun cpp tests |
1 similar comment
rerun cpp tests |
run java8 tests |
1 similar comment
run java8 tests |
run java8 tests |
run integration tests |
2 similar comments
run integration tests |
run integration tests |
@codelipenghui this needs to be PIP-47? |
@sijie Yes, it should be PIP-47 |
4c3e590
to
bb18d12
Compare
run java8 tests |
run java8 tests |
run cpp tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codelipenghui implementation looks good to me in general. however I don't think we should let user specify the total hash range. we should pick a fixed hash range (e.g. 0~66536), and provide util functions for consumers to split the hash range based on their requirements. please take a look at my detailed comment below.
* If new consumer use conflict hash range with exits consumers, new consumer | ||
* will be rejected. | ||
*/ | ||
EXCLUSIVE_HASH_RANGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably just call it STICKY
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, STICKY
is more simpler
|
||
message KeySharedMeta { | ||
required KeySharedMode keySharedMode = 1; | ||
required int32 totalHashRange = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should allow users to configure thee totalHashRange. The hash range should be fixed (e.g. 0.0 ~ 1.0, Long.MIN_VALUE ~ Long.MAX_VALUE, or Integer.MIN_VALUE to Integer.MAX_VALUE, or 0 ~ 66536). Just pick up a range, and let consumer specify the hash range.
Allowing user configuring the totalHashRange
introduces inconsistent across consumers of a subscription. people can make mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i will fix by using a fixed range(0-65536), current total range for key shared subscription is also (0-65536)
return end; | ||
} | ||
|
||
public Range intersect(Range range) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guava has a Range
class. You might consider reusing its class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pulsar client api module do not add guava dependency, shall we add the guava dependency?
d750988
to
a39e2e5
Compare
@sijie I have addressed you comments except the new Range class in pulsar client api module, because currently the pulsar client api module do not add guava dependency, considering to keep api module thin, so i add the new Range class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codelipenghui lgtm. this is a great addon to key_shared subscription.
I left one minor comment.
@@ -250,6 +255,16 @@ message AuthData { | |||
optional bytes auth_data = 2; | |||
} | |||
|
|||
enum KeySharedMode { | |||
autoSplit = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use upper case for enums?
AUTO_SPLIT = 0;
STICKY = 1;
@codelipenghui can you update the enum? |
@sijie I can update it tomorrow, my mac book has some problems with docker environment, can not build the proto. need to build them on another machine 😊 |
run java8 tests |
@sijie I have updated the enum in PulsarApi.proto |
Motivation
Introduce sticky consumer, users can enable it by
Modifications
Add a new consumer selector named HashRangeExclusiveStickyKeyConsumerSelector to support sticky consumer.
This change added tests and can be verified as follows:
Add new unit tests.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation