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

[fix] [doc] Update BookKeeper metadataServiceUri doc #20909

Conversation

hangc0276
Copy link
Contributor

Fixes #xyz

Main Issue: #xyz

PIP: #xyz

Motivation

If we want to use PulsarMetadataClientDriver in BookKeeper server, the document in conf/bookkeeper.conf is confused and it's hard for users to configure the right metadataServiceUri format.

Modifications

Add a document in conf/bookkeeper.conf to guide users to configure the right metadataServiceUri format.

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 the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@hangc0276 hangc0276 self-assigned this Jul 31, 2023
@hangc0276 hangc0276 added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jul 31, 2023
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Jul 31, 2023
@hangc0276 hangc0276 added this to the 3.1.0 milestone Jul 31, 2023
# - metadataServiceUri=zk+hierarchical://my-zk-1:2181/ledgers
# - metadataServiceUri=etcd+hierarchical:http://my-etcd:2379
# - metadataServiceUri=metadata-store:zk:my-zk-1:2281
# If you use metadata-store configuration, you need to configure following items in JVM option:
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
# If you use metadata-store configuration, you need to configure following items in JVM option:
# If you want to use the Apache Pulsar metadata-store driver, you need to configure the following items in the JVM option:

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should explain why we must set the metadata-store prefix. WDYT? :)

@Technoboy- Technoboy- closed this Aug 21, 2023
@Technoboy- Technoboy- reopened this Aug 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2023

Codecov Report

Merging #20909 (1597362) into master (ca01447) will decrease coverage by 1.38%.
Report is 31 commits behind head on master.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20909      +/-   ##
============================================
- Coverage     73.14%   71.76%   -1.38%     
+ Complexity    32221    32051     -170     
============================================
  Files          1874     1875       +1     
  Lines        139379   144556    +5177     
  Branches      15328    16217     +889     
============================================
+ Hits         101942   103741    +1799     
- Misses        29352    32568    +3216     
- Partials       8085     8247     +162     
Flag Coverage Δ
inttests 24.26% <ø> (+0.06%) ⬆️
systests 25.39% <ø> (+0.19%) ⬆️
unittests 71.48% <50.00%> (-0.92%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...pulsar/sql/presto/PulsarSqlSchemaInfoProvider.java 68.00% <0.00%> (ø)
...g/apache/pulsar/sql/presto/PulsarRecordCursor.java 72.18% <100.00%> (ø)

... and 103 files with indirect coverage changes

@Technoboy- Technoboy- modified the milestones: 3.1.0, 3.2.0 Aug 21, 2023
@Technoboy- Technoboy- merged commit 3eb9610 into apache:master Aug 21, 2023
95 of 99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants