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

[Improve][Standalone] Standalone Add param of --metadata-url for runing with metadata #17077

Merged
merged 5 commits into from
Sep 26, 2022
Merged

[Improve][Standalone] Standalone Add param of --metadata-url for runing with metadata #17077

merged 5 commits into from
Sep 26, 2022

Conversation

liangyuanpeng
Copy link
Contributor

Motivation

Standalone support running with metadata, like etcd, And rocksdb still by default.

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: Yes, Standalone support running with metadata store, but rocksdb metadataStore by default.

Documentation

Need to update docs?

  • [] doc-required

  • doc-not-needed

  • doc

  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 12, 2022
@liangyuanpeng liangyuanpeng changed the title Standalone Add param of --metadata-url for runing with metadata [Feature][Standalone] Standalone Add param of --metadata-url for runing with metadata Aug 12, 2022
@liangyuanpeng liangyuanpeng changed the title [Feature][Standalone] Standalone Add param of --metadata-url for runing with metadata [Improve][Standalone] Standalone Add param of --metadata-url for runing with metadata Aug 12, 2022
@codelipenghui
Copy link
Contributor

@liangyuanpeng Any reason to support using separate metadata for standalone? The standalone is designed for development, not for production.

@nodece
Copy link
Member

nodece commented Aug 17, 2022

I think @liangyuanpeng wants to use other metadata.

tisonkun
tisonkun requested changes Aug 17, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Please update the description according to the placeholder.

pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java Outdated
Comment on lines 441 to 447
private void startBookieWithMetadataStore() throws Exception {
if(StringUtils.isBlank(metadataStoreUrl)){
log.info("Starting BK with RocksDb metadata store");
metadataStoreUrl = "rocksdb://" + Paths.get(metadataDir).toAbsolutePath();
}else{
log.info("Starting BK with metadata store:",metadataStoreUrl);
}
Copy link
Member

Choose a reason for hiding this comment

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

Please format this code block according to our code style settings. Briefly, add necessary blanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review and i'm busy for otherthing, pick it up in the next week maybe.

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented Aug 18, 2022 •
edited
Loading

@liangyuanpeng Any reason to support using separate metadata for standalone? The standalone is designed for development, not for production.

@codelipenghui

Thanks for your reply, Yes, Standalone is more used to development and we can use other metadata to development for check the metadata after support running other metadata. Actually, i'm really do it, Base on etcd metadata store for write the dev code and not zookeeper.

@liangyuanpeng
Copy link
Contributor Author

@codelipenghui @tisonkun PTAL.

tisonkun
tisonkun approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Minor comments inline.

pulsar-broker/src/main/java/org/apache/pulsar/PulsarStandalone.java Outdated
@@ -52,6 +52,7 @@
import org.apache.pulsar.packages.management.storage.filesystem.FileSystemPackagesStorageProvider;
import org.apache.pulsar.zookeeper.LocalBookkeeperEnsemble;


Copy link
Member

Choose a reason for hiding this comment

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

Revert

@liangyuanpeng liangyuanpeng force-pushed the Standalone_with_metadata branch from a687b36 to c3f1ca0 Compare September 8, 2022 15:24
@tisonkun
Copy link
Member

@liangyuanpeng Pulsar CI has been refactored a lot. You can merge the latest master to run CI tasks.

eolivelli
eolivelli approved these changes Sep 12, 2022
View reviewed changes
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

@liangyuanpeng
Copy link
Contributor Author

@codelipenghui PTAL,Thanks.

@nodece
Copy link
Member

nodece commented Sep 26, 2022

Ping @codelipenghui, could you review this PR?

Jason918
Jason918 approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 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 3e51af5 into apache:master Sep 26, 2022
merlimat pushed a commit that referenced this pull request Oct 20, 2022
@liangyuanpeng @merlimat
[Improve][Standalone] Standalone Add param of --metadata-url for runi…
f0290fe 
…ng with metadata (#17077)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisonkun tisonkun tisonkun approved these changes

@nodece nodece nodece approved these changes

@eolivelli eolivelli eolivelli approved these changes

@Jason918 Jason918 Jason918 approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@merlimat merlimat Awaiting requested review from merlimat

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@liangyuanpeng liangyuanpeng

Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liangyuanpeng @codelipenghui @nodece @tisonkun @Jason918 @eolivelli