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

Add streaming dispatcher. #9056

Merged
merged 8 commits into from
Feb 2, 2021

Conversation

MarvinCai
Copy link
Contributor

Related to #3804

Motivation

Trying to streamline the dispatcher's read requests to manager ledger instead of micro batch.

Modifications

Created a StreamingEntryReader that can streamline read request to managed ledger.
Created StreamingDispatcher interface with necessary method to interact with StreamingEntryReader.
Created PersistentStreamingDispatcherSingleActive/MultipleConsumer that make use of StreamingEntryReader to read entries from managed ledger.
Add config to use streaming dispatcher.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)
This change added tests and can be verified as follows:
Add unit tests.

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

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • 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

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@@ -1915,6 +1920,21 @@ void notifyCursors() {
}
}

void notifyWaitingEntryCallBack() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void notifyWaitingEntryCallBack() {
void notifyWaitingEntryCallBacks() {

Copy link
Contributor Author

@MarvinCai MarvinCai Jan 6, 2021

Choose a reason for hiding this comment

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

done.

category = CATEGORY_SERVER,
doc = "Whether to use streaming read dispatcher."
)
private boolean streamingDispatch = false;
Copy link
Member

Choose a reason for hiding this comment

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

Can you mark this feature as a preview feature?

Copy link
Contributor Author

@MarvinCai MarvinCai Jan 6, 2021

Choose a reason for hiding this comment

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

updated the doc.

@@ -667,6 +667,12 @@
)
private boolean preciseDispatcherFlowControl = false;

@FieldContext(
category = CATEGORY_SERVER,
doc = "Whether to use streaming read dispatcher."
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to add more documentation about this feature.

public class PersistentStreamingDispatcherMultipleConsumers extends PersistentDispatcherMultipleConsumers
implements StreamingDispatcher {

private StreamingEntryReader streamingEntryReader = new StreamingEntryReader((ManagedCursorImpl) cursor,
Copy link
Member

Choose a reason for hiding this comment

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

final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@sijie sijie added the doc-required Your PR changes impact docs and you will update later. label Dec 30, 2020
@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@MarvinCai MarvinCai requested a review from sijie January 8, 2021 17:49
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@MarvinCai Overall looks good to me. Can you add more tests to the newly introduced classes?

*/
@Slf4j
@RequiredArgsConstructor
public class StreamingEntryReader implements AsyncCallbacks.ReadEntryCallback, WaitingEntryCallBack {
Copy link
Member

Choose a reason for hiding this comment

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

@MarvinCai Can you write a test case for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's StreamingEntryReaderTests class, it's folded by github.

@sijie
Copy link
Member

sijie commented Jan 14, 2021

@codelipenghui @hangc0276 Can you review this pull request?

@MarvinCai MarvinCai requested a review from sijie January 16, 2021 17:50
@sijie
Copy link
Member

sijie commented Jan 20, 2021

@codelipenghui Can you review this?

@sijie
Copy link
Member

sijie commented Feb 1, 2021

@MarvinCai Can you rebase this pull request?

@codelipenghui Can you review it?

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@MarvinCai
Copy link
Contributor Author

MarvinCai commented Feb 1, 2021

@sijie Rebased to newer version of master and resolved conflict.
Also I saw original Dispatcher logic has some changes related to Transaction, will have separate pr to update streaming Dispatcher if needed to be compatible with transaction.

@MarvinCai
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit 8cfaf48 into apache:master Feb 2, 2021
@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-required Your PR changes impact docs and you will update later. labels Apr 9, 2021
eolivelli pushed a commit that referenced this pull request May 13, 2021
Related to  #3804

Trying to streamline the dispatcher's read requests to manager ledger instead of micro batch.

Created a StreamingEntryReader that can streamline read request to managed ledger.
Created StreamingDispatcher interface with necessary method to interact with StreamingEntryReader.
Created PersistentStreamingDispatcherSingleActive/MultipleConsumer that make use of StreamingEntryReader to read entries from managed ledger.
Add config to use streaming dispatcher.

(cherry picked from commit 8cfaf48)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants