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

[feat][broker] PIP-157 Bucketing topic metadata to allow more topics per namespace #16901

Closed
wants to merge 8 commits into from

Conversation

andrasbeni
Copy link
Contributor

@andrasbeni andrasbeni commented Aug 1, 2022

Fixes #15254

Motivation

The number of topics in a namespace is limited by ZooKeeper when it is used as the local store. This is because listing topics requires all topic names to fit in ZK's buffer. By introducing an intermediate layer between the namespace node and topics, a namespace can have more topics.

Modifications

  • Add new namespace policy number_of_buckets defaulting to 1 (i.e. no buckets)
  • Calculate topics' persistence name based on the number of buckets in the namespace

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for creating topics in a bucketed namespace and producing to / consuming from it

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

If yes was chosen, please highlight the changes

  • 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

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

Matching PR in forked repository

PR in forked repository: andrasbeni#4

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Aug 1, 2022
@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

Comment on lines +59 to +64
CompletableFuture<List<String>> childrenOfNamespace = store.getChildren(path);
CompletableFuture<List<String>> topics = childrenOfNamespace.thenCompose(children ->
!children.isEmpty() && children.get(0).startsWith(TopicName.BUCKET_ID_PERFIX)
? children.stream().map(bucket -> store.getChildren(path + "/" + bucket))
.collect(FutureUtil.toList())
: CompletableFuture.completedFuture(children));
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks identical to L76-81, could they share a common helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. I did not realize this. I extracted the common part into a method.

return pulsar.getPulsarResources().getTopicResources().listPersistentTopicsAsync(namespaceName);
return pulsar.getPulsarResources().getTopicResources().listPersistentTopicsAsync(namespaceName)
.thenApply(x -> {
LOG.info("getListOfPersistentTopics: " + x);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be info?

(also x -> topics)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it needs to be logged at all. Reverted this part.

@codelipenghui codelipenghui added this to the 2.12.0 milestone Aug 4, 2022
@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun pulsar-ci-checks-completed

@andrasbeni
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@andrasbeni
Copy link
Contributor Author

/pulsarbot run-failure-checks

@andrasbeni andrasbeni closed this Sep 14, 2022
@andrasbeni andrasbeni reopened this Sep 14, 2022
@andrasbeni
Copy link
Contributor Author

@merlimat , please take a look

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 25, 2022
@andrasbeni
Copy link
Contributor Author

@merlimat, I don't think I will have time to work on this anytime soon. Feel free to take over if this is still relevant.

@github-actions github-actions bot removed the Stale label Mar 28, 2023
@RobertIndie RobertIndie modified the milestones: 3.0.0, 3.1.0 Apr 11, 2023
@andrasbeni andrasbeni closed this May 1, 2023
@tisonkun
Copy link
Member

tisonkun commented May 1, 2023

Thanks for your contribution @andrasbeni!

It seems the community doesn't give this PR a review in time. I add this PR in my backlog to see if it's still relevant. Perhaps ping you for reviewing if so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required Your PR changes impact docs and you will update later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PIP-157: Bucketing topic metadata to allow more topics per namespace
5 participants