Support dlog bookie client configuration via pass through config #15763
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaeljmarshall
added
doc
michaeljmarshall
requested review from
codelipenghui,
dave2wave,
eolivelli,
lhotari,
merlimat and
zymap
Anonymitaet
reviewed
May 25, 2022
| # you can add other configuration options for the BookKeeper client | ||
| # by prefixing them with bookkeeper_ | ||
| # You can add other configuration options for the BookKeeper client | ||
| # by prefixing them with "bookkeeper_". These configurations will be applied |
Member
There was a problem hiding this comment.
Suggested change
| # by prefixing them with "bookkeeper_". These configurations will be applied | |
| # by prefixing them with "bookkeeper_". These configurations are applied |
Write in the simple present tense as much as possible if you are covering facts that were, are, and forever shall be true. https://docs.google.com/document/d/1lc5j4RtuLIzlEYCBo97AC8-U_3Erzs_lxpkDuseU0n4/edit#bookmark=id.e8uqh1awkcnp
|
|
||
| # When using BookKeeperPackagesStorageProvider, you can configure the | ||
| # bookkeeper client by prefixing configurations with "bookkeeper_". | ||
| # This config will apply to managed ledger bookkeeper clients, as well. |
Member
There was a problem hiding this comment.
Suggested change
| # This config will apply to managed ledger bookkeeper clients, as well. | |
| # This configuration applies to managed ledger bookkeeper clients as well. |
Member
Author
|
@Anonymitaet - thank you for your feedback. Making this a draft temporarily. I will include some additional work tomorrow. |
Member
Author
|
This PR is superseded by #15818. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Enable pass through configuration support for the bookkeeper clients created by
DistributedLogandBookKeeperPackagesStorage. This support is already present for the managed ledger bookkeeper client, which takes allbookkeeper_prefixed configs. In order to simplify configuration, I reused thebookkeeper_prefix since it's reasonable to assume that these configs will be the same.Modifications
PulsarConfigurationUtilsclass to reduce code duplication for mapping prefixed config classes into other config classes. This class is not available in thepulsar-package-managementmodule, so the code is copied. I don't think we should expose this code in the client jars.BookKeeperPackagesStorageConfigurationto expose the underlyingpropertiesobject.broker.confand thefunctions_worker.ymlfiles.Verifying this change
I added tests to ensure that the new methods work as expected.
Does this pull request potentially affect one of the following parts:
This is a backwards compatible change. The only nuance is that the package management dlog bookie client will inherit the same configs that the broker already uses for the bookie client. I think this is preferable.