Conversation
The "Starting Bookie server" log line introduced in apache#4768 was meant to print only the overridden settings, but it compared every key against an empty ServerConfiguration. The defaults are not stored in the configuration object, they are hard-coded inside the getters, so the lookup returned null for every key and all of them were reported as overrides. When the configuration is shared with other components, for example Pulsar standalone loading the broker settings into the same properties, hundreds of unrelated entries were printed. Compute the overrides by invoking every ServerConfiguration getter on both the actual configuration and an empty one, through a subclass that records the property keys each getter reads. When the two results differ, the recorded keys that are explicitly set are reported with their raw values. Keys that no getter reads are left out.
There was a problem hiding this comment.
🟢 Approval recommended
Changes are fully reviewed, tested, and introduce no unresolved blocking issues.
Pull request overview
Updates bookie startup logging to report only explicitly configured settings that differ from built-in defaults.
Changes:
- Compares configuration getters against an empty configuration while recording accessed keys.
- Excludes unrelated and unknown properties.
- Adds tests covering defaults, overrides, fallback keys, unknown keys, and invalid values.
File summaries
| File | Description |
|---|---|
bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieServerOverriddenConfigTest.java |
Tests override detection behavior and edge cases. |
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java |
Implements getter-based override detection and key recording. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Descriptions of the changes in this PR:
Follow-up to #4768.
Motivation
The
Starting Bookie serverlog line introduced in #4768 was meant to print only the settings overridden from the defaults, but in practice it printed every key ever set on the configuration. It compared each key against an emptyServerConfiguration, and the defaults are not stored in the configuration object: they are hard-coded inside the getters (e.g.getInt(BOOKIE_PORT, 3181)). The lookup therefore returnednullfor every key, and all of them were reported as overrides.This is especially noisy when the configuration is shared with other components. Pulsar standalone loads the broker settings into the same properties object, so the bookie printed hundreds of unrelated broker entries at startup.
Changes
BookieServer.overriddenConfig()now invokes every publicget*/is*getter ofServerConfigurationon both the actual configuration and an empty one, through a small subclass that records which property keys each getter reads. When the two results differ, the recorded keys that are explicitly set in the configuration are reported with their raw values.ServerConfigurationgetter reads are not reported. This drops the unrelated entries (e.g. Pulsar broker settings), but also component-owned keys such asdbStorage_*.DbLedgerStoragealready logs its cache sizes at startup.CompositeConfigurationimplementsgetList/getStringArrayby reading its child configurations directly, bypassinggetPropertyInternal, so the recording subclass overridesgetListas well. Without it, list-valued settings such asledgerDirectorieswere missed.BookieServerOverriddenConfigTestcovering values equal to the defaults, changed values, fallback keys, unknown keys and values rejected by a getter.Example output for a bookie started by
BookieClientTest; only the settings the test configuration explicitly changes are listed: