shauryachats commented on code in PR #19064:
URL: https://github.com/apache/pinot/pull/19064#discussion_r3771271029
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##########
@@ -199,21 +206,58 @@ void markQueryFailed(ServerRoutingInstance
serverRoutingInstance, Exception exce
}
/**
- * NOTE: the server might not be hit by the query. Only fail the query if
the query was sent to the server and the
- * server hasn't responded yet.
+ * Marks a server as unavailable while the query is in flight - its Netty
channel went inactive or a request write
+ * to it failed. This is a genuine server-unavailability event that is
eligible for {@code skipUnavailableServers}.
+ * NOTE: the server might not have been hit by this query. Only acts if the
query was sent to the server and the
+ * server has not responded yet.
*/
- void markServerDown(ServerRoutingInstance serverRoutingInstance, Exception
exception) {
- ServerResponse serverResponse = _responseMap.get(serverRoutingInstance);
- if (serverResponse != null && serverResponse.getDataTable() == null) {
- markQueryFailed(serverRoutingInstance, exception);
+ boolean markServerUnavailable(ServerRoutingInstance serverRoutingInstance,
Exception exception) {
+ if (!shouldActOnServerDown(serverRoutingInstance)) {
+ return false;
+ }
+ if (_skipUnavailableServers) {
+ // Best-effort: degrade to partial results. Record the down server so
the failure detector can quarantine it from
+ // routing, but do NOT set the query-global exception/status (no
BROKER_REQUEST_SEND error, query stays
+ // COMPLETED)
+ if (_countedDownServers.add(serverRoutingInstance)) {
+ _failedServer = serverRoutingInstance;
+ _countDownLatch.countDown();
Review Comment:
Possible TOCTOU race: checking
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##########
@@ -199,21 +206,58 @@ void markQueryFailed(ServerRoutingInstance
serverRoutingInstance, Exception exce
}
/**
- * NOTE: the server might not be hit by the query. Only fail the query if
the query was sent to the server and the
- * server hasn't responded yet.
+ * Marks a server as unavailable while the query is in flight - its Netty
channel went inactive or a request write
+ * to it failed. This is a genuine server-unavailability event that is
eligible for {@code skipUnavailableServers}.
+ * NOTE: the server might not have been hit by this query. Only acts if the
query was sent to the server and the
+ * server has not responded yet.
*/
- void markServerDown(ServerRoutingInstance serverRoutingInstance, Exception
exception) {
- ServerResponse serverResponse = _responseMap.get(serverRoutingInstance);
- if (serverResponse != null && serverResponse.getDataTable() == null) {
- markQueryFailed(serverRoutingInstance, exception);
+ boolean markServerUnavailable(ServerRoutingInstance serverRoutingInstance,
Exception exception) {
+ if (!shouldActOnServerDown(serverRoutingInstance)) {
+ return false;
+ }
+ if (_skipUnavailableServers) {
+ // Best-effort: degrade to partial results. Record the down server so
the failure detector can quarantine it from
+ // routing, but do NOT set the query-global exception/status (no
BROKER_REQUEST_SEND error, query stays
+ // COMPLETED)
+ if (_countedDownServers.add(serverRoutingInstance)) {
+ _failedServer = serverRoutingInstance;
+ _countDownLatch.countDown();
Review Comment:
Possible TOCTOU race: checking
--
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]
