return exception when unavailable segments on empty broker response #7823
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7823 +/- ##
============================================
- Coverage 71.70% 70.53% -1.17%
- Complexity 4089 4091 +2
============================================
Files 1579 1579
Lines 80832 80841 +9
Branches 12010 12010
============================================
- Hits 57957 57025 -932
- Misses 18978 19933 +955
+ Partials 3897 3883 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -459,7 +459,13 @@ private BrokerResponseNative handleSQLRequest(long requestId, String query, Json | |||
| if (offlineBrokerRequest == null && realtimeBrokerRequest == null) { | |||
| LOGGER.info("No server found for request {}: {}", requestId, query); | |||
| _brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.NO_SERVER_FOUND_EXCEPTIONS, 1); | |||
| return BrokerResponseNative.EMPTY_RESULT; | |||
| BrokerResponseNative brokerResponse = BrokerResponseNative.EMPTY_RESULT; | |||
| if (numUnavailableSegments > 0) { | |||
There was a problem hiding this comment.
I think the intention of the PR #7397 was to always add this exception to BrokerResponse if numUnavailableSegments is > 0 and this case was missed ?
So may be we just do this once by moving the code at line 512 to proper place that covers all scenarios unconditionally ?
There was a problem hiding this comment.
It's a little tough to do with the way the code is structured. We can't return early when there's segments missing because many use cases still want partial results if possible. And there are several places in this function that short-circuit and return their own response before adding previous exceptions.
But I've moved the code up to create a list of exceptions we can add to. And I've added the missing segments exception to the timeout response as well.
| List<QueryProcessingException> exceptions = new ArrayList<>(); | ||
| if (numUnavailableSegments > 0) { | ||
| exceptions.add(new QueryProcessingException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE, | ||
| String.format("%d segments %s unavailable", numUnavailableSegments, unavailableSegments))); | ||
| } |
There was a problem hiding this comment.
Here we can store a list of ProcessingException which is easier to manage
| List<QueryProcessingException> exceptions = new ArrayList<>(); | |
| if (numUnavailableSegments > 0) { | |
| exceptions.add(new QueryProcessingException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE, | |
| String.format("%d segments %s unavailable", numUnavailableSegments, unavailableSegments))); | |
| } | |
| List<ProcessingException> exceptions = new ArrayList<>(); | |
| if (numUnavailableSegments > 0) { | |
| exceptions.add(QueryException.getException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE, | |
| String.format("%d segments %s unavailable", numUnavailableSegments, unavailableSegments))); | |
| } |
There was a problem hiding this comment.
The only downside of this is under the hood, getException does
String errorType = processingException.getMessage();
ProcessingException copiedProcessingException = processingException.deepCopy();
copiedProcessingException.setMessage(errorType + ":\n" + errorMessage);
return copiedProcessingException;
which is setting the error message to "message": "null:\n1 segments [meetupRsvp_REALTIME_1637786040816_0__0__1637786041067] unavailable". The null:\n looks weird. I could make that code not do that on nulls, but I have no idea what else changes at that point.
| BrokerResponseNative brokerResponse = BrokerResponseNative.EMPTY_RESULT; | ||
| brokerResponse.addToExceptions(exceptions); | ||
| return brokerResponse; |
There was a problem hiding this comment.
(major) We cannot modify the constant broker response. Here we need to create a new one. If we apply the suggested change above, we can directly use the exception constructor
| BrokerResponseNative brokerResponse = BrokerResponseNative.EMPTY_RESULT; | |
| brokerResponse.addToExceptions(exceptions); | |
| return brokerResponse; | |
| return new BrokerResponseNative(exceptions); |
| BrokerResponseNative brokerResponse = | ||
| new BrokerResponseNative(QueryException.getException(QueryException.BROKER_TIMEOUT_ERROR, errorMessage)); | ||
| brokerResponse.addToExceptions(exceptions); | ||
| return brokerResponse; |
There was a problem hiding this comment.
This can also be simplified:
| BrokerResponseNative brokerResponse = | |
| new BrokerResponseNative(QueryException.getException(QueryException.BROKER_TIMEOUT_ERROR, errorMessage)); | |
| brokerResponse.addToExceptions(exceptions); | |
| return brokerResponse; | |
| exceptions.add(QueryException.getException(QueryException.BROKER_TIMEOUT_ERROR, errorMessage)); | |
| return new BrokerResponseNative(exceptions); |
Description
This fixes #7811. In #7264 we added exceptions when not all segments are available, but this was one case that was missed. Specifically if all servers are unavailable, both realtime and offline requests become null, but there are still unavailable segments. This adds the exception to the broker response.
There's no unit tests here which makes it quite difficult for me to setup something. But I tested locally by running the realtime quickstart
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation
This will get encompassed whenever the new error code documentation is done.