-
Notifications
You must be signed in to change notification settings - Fork 93
[server][controller][vpj] Add PubSubConsumerAdapter::subscribe with PubSubPosition parameter #1489
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
[server][controller][vpj] Add PubSubConsumerAdapter::subscribe with PubSubPosition parameter #1489
Conversation
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubPosition.java
Outdated
Show resolved
Hide resolved
1ef6af9
to
b6f2e78
Compare
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.
Overall looks good. Thanks for the detailed PR description and help me to get understanding of the purpose. I left a few comments but overall it is in good shape to me.
...s/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/SharedKafkaConsumer.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/annotation/UnderDevelopment.java
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/annotation/VisibleForTesting.java
Show resolved
Hide resolved
.../main/java/com/linkedin/venice/pubsub/adapter/kafka/consumer/ApacheKafkaConsumerAdapter.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubPosition.java
Outdated
Show resolved
Hide resolved
Temp changes Add magic positions Add UTs Add tests
b6f2e78
to
79df243
Compare
79df243
to
7a012c7
Compare
Thanks, @sixpluszero! |
.../main/java/com/linkedin/venice/pubsub/adapter/kafka/consumer/ApacheKafkaConsumerAdapter.java
Show resolved
Hide resolved
.../main/java/com/linkedin/venice/pubsub/adapter/kafka/consumer/ApacheKafkaConsumerAdapter.java
Show resolved
Hide resolved
.../main/java/com/linkedin/venice/pubsub/adapter/kafka/consumer/ApacheKafkaConsumerAdapter.java
Show resolved
Hide resolved
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.
Good comments! I left a recommendation for keeping the reference equal in one place.
Add PubSubConsumerAdapter::subscribe with PubSubPosition parameter
Introduce a new overload of
subscribe
inPubSubConsumerAdapter
that accepts aPubSubPosition
. This change is aimed at supporting non-numeric offsets. This allowsconsumers to specify positions such as
EARLIEST
,LATEST
, or an implementation-specificposition type. Implementors must map this
PubSubPosition
to the appropriate offset inthe underlying pub-sub system.
Enhancements to PubSub Consumer Functionality:
PubSubConsumerAdapter:
Added a new
subscribe
method that usesPubSubPosition
to determine the startingoffset for consumption. This method is marked with the
UnderDevelopment
annotation.(
PubSubConsumerAdapter.java
)PubSubPosition Updates:
Added
EARLIEST
andLATEST
positions to thePubSubPosition
interface.(
PubSubPosition.java
)ApacheKafkaConsumerAdapter Implementation:
Implemented the new
subscribe
method to handlePubSubPosition
.New Annotations:
UnderDevelopment:
Added a new annotation to indicate that an API is under development and not yet stable.
(
UnderDevelopment.java
)VisibleForTesting:
Added a new annotation to indicate that a method, class, or field is more visible than
necessary for testing purposes. (
VisibleForTesting.java
)How was this PR tested?
UT and E2ETest
Does this PR introduce any user-facing changes?