abhishekrb19 commented on code in PR #19950:
URL: https://github.com/apache/druid/pull/19950#discussion_r3764147538
##########
server/src/main/java/org/apache/druid/server/coordinator/loading/HttpLoadQueuePeon.java:
##########
@@ -168,17 +168,28 @@ private SegmentLoadingCapabilities
fetchSegmentLoadingCapabilities()
).get();
if (HttpServletResponse.SC_NOT_FOUND == responseHandler.getStatus()) {
- int batchSize = config.getBatchSize() == null ? 1 :
config.getBatchSize();
- SegmentLoadingCapabilities defaultCapabilities = new
SegmentLoadingCapabilities(batchSize, batchSize);
+ SegmentLoadingCapabilities defaultCapabilities =
getDefaultLoadingCapabilities();
log.warn(
"Historical capabilities endpoint not found at URL[%s]. Using
default values[%s].",
segmentLoadingCapabilitiesURL,
defaultCapabilities
);
return defaultCapabilities;
} else if (HttpServletResponse.SC_OK != responseHandler.getStatus()) {
- log.makeAlert("Received status[%s] when fetching loading capabilities
from server[%s]", responseHandler.getStatus(), serverId);
- throw new RE("Received status[%s] when fetching loading capabilities
from server[%s]", responseHandler.getStatus(), serverId);
+ // A single unhealthy server (e.g. one returning 503) must not prevent
this peon from being
+ // created. Peon construction happens inside
LoadQueueTaskMaster.resetPeonsForNewServers, which
+ // is the first thing PrepareBalancerAndLoadQueues (the first duty in
HistoricalManagementDuties)
+ // does; throwing here would abort the entire duty group for every
server in the cluster and
+ // stall all segment loading/balancing. Instead, alert and fall back
to default capabilities so
+ // this server is still managed and the rest of the run proceeds.
+ SegmentLoadingCapabilities defaultCapabilities =
getDefaultLoadingCapabilities();
Review Comment:
>While it might make sense to ignore failures for a single server, the new
flow would ignore failures even if all historicals failed to return
capabilities.
We should probably only support ignoring a small fraction of failures.
@kfaraz thanks for taking a look. I gave this comment some thought. Is the
concern around observability or something behavioral? Even in the old flow,
there were alerts/errors surfaced in the duty runs due to failures here - lack
of cluster-wide segment assignment and other
second order effects were only observed as a side effect of that. The new
flow logs the same set of alerts/errors, and we could make them louder if it's
needed.
I think doing it for n% of servers would make sense, but I think the
failure mode would be somewhat confusing and would still have the downsides of
the old behavior. Please let me know what you think.
Also, I think having this self-heal behavior would be good
-https://github.com/apache/druid/pull/19950#discussion_r3758121929
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
