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

Setup initial namespaces with MetadataStore #10612

Merged
merged 3 commits into from
May 21, 2021

Conversation

fantapsody
Copy link
Contributor

Motivation

Refactor the initial namespaces setup command to use the new MetadataStore API.

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

  • Refactored PulsarInitialNamespaceSetup command to use the new MetadataStore API.

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as ClusterMetadataSetupTest.

(or)

This change added tests and can be verified as follows:

  • Added unit tests for the PulsarInitialNamespaceSetup command

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

  • Does this pull request introduce a new feature? (no)

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

We may have a problem with the new test case as the new refactored method may call System.exit, breaking the execution of the JVM.

I would be better to be able to pass to the PulsarInitialNamespaceSetup.main() function a callback to be executed in case of failure and use that version for tests

PulsarInitialNamespaceSetup.main(arguments,  (exitCode) -> {
    fail the test
});

@fantapsody
Copy link
Contributor Author

LGTM

We may have a problem with the new test case as the new refactored method may call System.exit, breaking the execution of the JVM.

I would be better to be able to pass to the PulsarInitialNamespaceSetup.main() function a callback to be executed in case of failure and use that version for tests

PulsarInitialNamespaceSetup.main(arguments,  (exitCode) -> {
    fail the test
});

@eolivelli Thanks for the idea! I modified the code in a similar way by moving the code from main to doMain and let doMain return a status code, please take another look.

@fantapsody fantapsody requested a review from eolivelli May 19, 2021 07:21
@codelipenghui codelipenghui added this to the 2.8.0 milestone May 20, 2021
@merlimat
Copy link
Contributor

@eolivelli @fantapsody has already addressed the comments

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label May 20, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit a672fc1 into apache:master May 21, 2021
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants