kfaraz commented on code in PR #19950:
URL: https://github.com/apache/druid/pull/19950#discussion_r3755130820
##########
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.
Also, I wonder if instead of using the default, we should try to use the
average of the successful responses. But maybe using the default is safer.
##########
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.
Review Comment:
```suggestion
// Do not stall further processing due to a single unhealthy server
// Raise an alert and use default capabilities
```
--
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]
