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.

Sign up for GitHub

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

Jump to bottom

KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation #7

Merged
merged 6 commits into from
Jun 8, 2023
Merged

KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation #7

merged 6 commits into from
Jun 8, 2023

Conversation

philipnee
Copy link
Owner

This is the implementation of the TopicMetadataRequestManager, responsible for handling topic metadata from partitionsFor and listTopics

The PR also refactors the current callback mechanism because I think the usage is rather inorganic. Now, user can pass in a BiConsumer to be triggered when the request is completed.

@kirktrue @lianetm

@philipnee philipnee force-pushed the consumer-refactor-metadata-request-manager branch from b7f2814 to eef8a9f Compare May 10, 2023 18:27
kirktrue
kirktrue requested changes May 10, 2023
View reviewed changes
Copy link
Collaborator

@kirktrue kirktrue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see how this plugs into the background thread and application event processor, specifically how it handles ensuring that duplicate API calls are handled appropriately.

Also, can we refactor TopicMetadataRequestManager and TopicMetadataFetcher so that they use the same core response handling logic?

...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
@philipnee philipnee force-pushed the consumer-refactor-metadata-request-manager branch from 8ae018e to 8616b93 Compare May 11, 2023 20:14
@philipnee
Copy link
Owner Author

Thanks for the review - @kirktrue @lianetm Addressed the comments

@philipnee
Copy link
Owner Author

I think I ommitted kirk's comments: Working on them ...

@philipnee philipnee force-pushed the consumer-refactor-metadata-request-manager branch from 8616b93 to 03ff808 Compare May 12, 2023 20:11
@philipnee
Copy link
Owner Author

@kirktrue @lianetm - I added some code around how ApplicationEventProcessor hook up with the request manager. The second PR will cover how it hook up with the client thread/API.

kirktrue
kirktrue reviewed May 30, 2023
View reviewed changes
Copy link
Collaborator

@kirktrue kirktrue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm getting lost with all of the futures and callbacks. I feel we may want to do some restructuring, renaming, or documenting to keep things straight.

clients/src/main/java/org/apache/kafka/clients/consumer/internals/CompletableRequest.java

import java.util.concurrent.CompletableFuture;

public class CompletableRequest<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some JavaDoc? I'm not totally sure when to use this class vs. using the future from the UnsentRequest.

clients/src/main/java/org/apache/kafka/clients/consumer/internals/NetworkClientDelegate.java Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Outdated Show resolved Hide resolved
...s/src/main/java/org/apache/kafka/clients/consumer/internals/TopicMetadataRequestManager.java Show resolved Hide resolved
...ain/java/org/apache/kafka/clients/consumer/internals/events/CompletableApplicationEvent.java Outdated Show resolved Hide resolved
@philipnee
metadata stuff
f09fd9a 
fix broken test

Add tests and fix response handler

cleaned up and addressed some of the comments from the PR

wip

wip

Testing the impl

Added tests

MetadataRequestManager and refactor the callbacks

Update MetadataRequestManager.java

Create MetadataRequestManager.java
@philipnee philipnee force-pushed the consumer-refactor-metadata-request-manager branch from 90ed7de to f09fd9a Compare June 1, 2023 17:43
@philipnee
Copy link
Owner Author

@kirktrue thanks for the review! Ready for another pass. Think I addressed your comments.

@kirktrue kirktrue merged commit 53ac65b into ctr-staging Jun 8, 2023
philipnee added a commit that referenced this pull request Jun 9, 2023
@philipnee @lianetm
KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation (#7)
1bfc4f6 
TopicMetadataRequestManager implementation

---------

Co-authored-by: Lianet Magrans <lmagrans@confluent.io>
kirktrue added a commit that referenced this pull request Jun 22, 2023
@kirktrue
KAFKA-14274, #7: introduce FetchRequestManager to integrate fetch int…
614e437 
…o new consumer threading refactor

This change introduces the FetchRequestManager that will be responsible for:

- Formatting fetch requests to the background thread
- Configuring the callback on fetch responses for the background thread

The response handler will collect the fetch responses from the broker and create CompletedFetch instances as is done in Fetcher. The foreground logic will decompress the data into a Record, which will then be deserialized into a ConsumerRecord for returning to the user.
philipnee added a commit that referenced this pull request Jul 19, 2023
@philipnee @lianetm
KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation (#7)
639e3f6 
TopicMetadataRequestManager implementation

---------

Co-authored-by: Lianet Magrans <lmagrans@confluent.io>
philipnee added a commit that referenced this pull request Sep 11, 2023
@philipnee @lianetm
KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation (#7)
33a221e 
TopicMetadataRequestManager implementation

---------

Co-authored-by: Lianet Magrans <lmagrans@confluent.io>
philipnee added a commit that referenced this pull request Sep 11, 2023
@philipnee @lianetm
KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation (#7)
3b7400b 
TopicMetadataRequestManager implementation

---------

Co-authored-by: Lianet Magrans <lmagrans@confluent.io>
philipnee added a commit that referenced this pull request Sep 13, 2023
@philipnee @lianetm
KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation (#7)
4b2a7f7 
TopicMetadataRequestManager implementation

---------

Co-authored-by: Lianet Magrans <lmagrans@confluent.io>
philipnee added a commit that referenced this pull request Sep 13, 2023
@philipnee @lianetm
KAFKA-14960: [Part I]TopicMetadataRequestManager Implementation (#7)
d236f1b 
TopicMetadataRequestManager implementation

---------

Co-authored-by: Lianet Magrans <lmagrans@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lianetm lianetm lianetm left review comments

@kirktrue kirktrue kirktrue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@philipnee @kirktrue @lianetm