Fix bug during initialization of HttpServerInventoryView #14517
Merged
Conversation
This file contains 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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
server/src/test/java/org/apache/druid/client/HttpServerInventoryViewTest.java
Fixed
Show resolved
Hide resolved
docs/operations/metrics.md
Outdated
Comment on lines
328
to
329
| |`segment/serverview/sync/healthy`|Sync status of the Broker with a segment-loading server such as a Historical or Peon. Emitted only when [HTTP-based server view](../configuration/index.md#segment-management) is enabled. This metric can be used in conjunction with `segment/serverview/sync/unstableTime` to debug slow startup of the Coordinator.|`server`, `tier`|1 for fully synced servers, 0 otherwise| | ||
| |`segment/serverview/sync/unstableTime`|Time in milliseconds for which the Broker has been failing to sync with a segment-loading server. Emitted only when [HTTP-based server view](../configuration/index.md#segment-management) is enabled.|`server`, `tier`|Not emitted for synced servers.| |
There was a problem hiding this comment.
Suggested change
| |`segment/serverview/sync/healthy`|Sync status of the Broker with a segment-loading server such as a Historical or Peon. Emitted only when [HTTP-based server view](../configuration/index.md#segment-management) is enabled. This metric can be used in conjunction with `segment/serverview/sync/unstableTime` to debug slow startup of the Coordinator.|`server`, `tier`|1 for fully synced servers, 0 otherwise| | |
| |`segment/serverview/sync/unstableTime`|Time in milliseconds for which the Broker has been failing to sync with a segment-loading server. Emitted only when [HTTP-based server view](../configuration/index.md#segment-management) is enabled.|`server`, `tier`|Not emitted for synced servers.| | |
| |`segment/serverview/sync/healthy`|Sync status of the Coordinator with a segment-loading server such as a Historical or Peon. Emitted only when [HTTP-based server view](../configuration/index.md#segment-management) is enabled. This metric can be used in conjunction with `segment/serverview/sync/unstableTime` to debug slow startup of the Coordinator.|`server`, `tier`|1 for fully synced servers, 0 otherwise| | |
| |`segment/serverview/sync/unstableTime`|Time in milliseconds for which the Coordinator has been failing to sync with a segment-loading server. Emitted only when [HTTP-based server view](../configuration/index.md#segment-management) is enabled.|`server`, `tier`|Not emitted for synced servers.| |
There was a problem hiding this comment.
Thanks for catching this! Did a bunch of copy-pastes and missed this.
| } | ||
|
|
||
| private void sync() | ||
| { | ||
| if (!startStopLock.awaitStarted(1, TimeUnit.MILLISECONDS)) { | ||
| log.info("Skipping sync() call for server[%s].", logIdentity); | ||
| log.info("Skipping sync for server[%s] as lifecycle has not started.", logIdentity); |
There was a problem hiding this comment.
these error messages are also a bit cryptic. I wonder if we can make them more meaningful. like what does lifecycle even mean?
There was a problem hiding this comment.
I am updating the message to say "syncer has not started yet" as the startStopLock denotes nothing but the lifecycle of the syncer itself.
Comment on lines
217
to
222
| ScheduledExecutors.scheduleAtFixedRate( | ||
| executor, | ||
| Duration.standardSeconds(60), | ||
| Duration.standardMinutes(5), | ||
| this::checkAndResetUnhealthyServers | ||
| ); |
There was a problem hiding this comment.
this could run with fixed delay instead of fixed rate.
Comment on lines
223
to
227
| ScheduledExecutors.scheduleAtFixedRate( | ||
| executor, | ||
| Duration.standardSeconds(30), | ||
| Duration.standardMinutes(1), | ||
| this::emitServerStatusMetrics |
There was a problem hiding this comment.
I just realized that one benefit of having a separate executor is that metrics get emitted even if thread pool is busy for other reasons. Thought could be done later.
There was a problem hiding this comment.
Yeah, true. If I am making other changes, I will move metric emission and the check and reset logic to the separate executor.
server/src/main/java/org/apache/druid/client/HttpServerInventoryView.java
Show resolved
Hide resolved
| } | ||
|
|
||
| return false; | ||
| final long unstableSeconds = getUnstableTimeMillis() / 1000; | ||
| final String message = StringUtils.format( |
There was a problem hiding this comment.
anything an admin can do when they see this message?
There was a problem hiding this comment.
Added an action in case of exceeding the maxUnstableDuration.
|
Thanks for the feedback, @abhishekagarwal87 . I have addressed your comments. |
abhishekagarwal87
approved these changes
Jul 6, 2023
10 tasks
sergioferragut
pushed a commit
to sergioferragut/druid
that referenced
this pull request
Jul 21, 2023
If a server is removed during `HttpServerInventoryView.serverInventoryInitialized`, the initialization gets stuck as this server is never synced. The method eventually times out (default 250s). Fix: Mark a server as stopped if it is removed. `serverInventoryInitialized` only waits for non-stopped servers to sync. Other changes: - Add new metrics for better debugging of slow broker/coordinator startup - `segment/serverview/sync/healthy`: whether the server view is syncing properly with a server - `segment/serverview/sync/unstableTime`: time for which sync with a server has been unstable - Clean up logging in `HttpServerInventoryView` and `ChangeRequestHttpSyncer` - Minor refactor for readability - Add utility class `Stopwatch` - Add tests and stubs
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.
Split from #14468
Bug
Description: If a server is removed during
HttpServerInventoryView.serverInventoryInitialized, the initialization gets stuck as this server is never synced. The method eventually times out (default 250s).Fix: Mark a server as stopped if it is removed.
serverInventoryInitializedonly waits for non-stopped servers to sync.Changes
HttpServerInventoryViewsegment/serverview/sync/healthy: A 1-0 metric that denotes if the server view is syncing properly with a serversegment/serverview/sync/unstableTime: tracks the time in millis for which sync with a server has been unstableHttpServerInventoryViewandChangeRequestHttpSyncerStopwatchClasses to review
ChangeRequestHttpSyncerHttpServerInventoryViewPending
Release note
This PR has: