Skip to content

Conversation

@MarvinCai
Copy link
Contributor

Fixes #8834

Motivation

Make dispatcher pluggable so we can support features like #8544 without changing default dispatcher.

Modifications

Moved Dispatcher interface and abstract classes to dispatcher packages to make package hierarchy a bit cleaner.
Add configuration for specifying customized dispatcher for persistent/nonpersistent + subtype combination.
Create util class to load customized dispatcher from NAR file.
Create factory class to supply dispatcher based on provided configuration.

Verifying this change

Existing test cases passed, added new unit tests for util class and dispatcher factory.

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 / no)
  • If yes, how is the feature documented? JavaDocs

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

Does this need to update doc? Please open an issue to track doc if needed.

+ " non-backlog consumers as well.")
private boolean dispatchThrottlingOnNonBacklogConsumerEnabled = false;

// <-- Pluggable dispatcher -->
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add the following configurations into the broker.conf and standalone.conf?

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 Thank you for putting this up!

But I think we should only have one DispatchFactory interface and it can be used for creating dispatchers for different subscription types. We shouldn't introduce that many configuration settings.

Also it might be worth writing a PIP for this effort.

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

@MarvinCai

The goal of making dispatcher pluggable is to avoid any computation and complexity in the dispatch path. So, we should target to make dispatching-path pluggable rather than making dispatcher pluggable. I thinking making dispatcher pluggable will add a lot of work for the user and will be complex. It will also make implementation simpler without adding configuration for each dispatcher as well. So, we should create a PIP for this feature and discuss it.
We were also working on the same feature proposal. so, if it's fine then we can submit the proposal and start the discussion.

@MarvinCai MarvinCai closed this Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make Pulsar Dispatcher Pluggable

4 participants