You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/09/07 21:22:16 UTC
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7397: [7264] Add Exception to Broker Response When Not All Segments Are Available (Partial Response)
Jackie-Jiang commented on a change in pull request #7397:
URL: https://github.com/apache/pinot/pull/7397#discussion_r703824504
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
##########
@@ -63,19 +63,27 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTrace) {
public static final int COMBINE_GROUP_BY_EXCEPTION_ERROR_CODE = 600;
public static final int QUERY_VALIDATION_ERROR_CODE = 700;
public static final int UNKNOWN_ERROR_CODE = 1000;
+ public static final int SEGMENTS_NOT_ONLINE_ERROR_CODE = 1001;
Review comment:
Let's not add error code larger than 1000. Also leave some space for future errors
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
##########
@@ -63,19 +63,27 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTrace) {
public static final int COMBINE_GROUP_BY_EXCEPTION_ERROR_CODE = 600;
public static final int QUERY_VALIDATION_ERROR_CODE = 700;
public static final int UNKNOWN_ERROR_CODE = 1000;
+ public static final int SEGMENTS_NOT_ONLINE_ERROR_CODE = 1001;
+ public static final int SEGMENTS_NOT_ACQUIRED_ERROR_CODE = 1002;
+ public static final int SERVERS_NO_RESPONSE_ERROR_CODE = 1003;
// NOTE: update isClientError() method appropriately when new codes are added
- public static final ProcessingException JSON_PARSING_ERROR = new ProcessingException(JSON_PARSING_ERROR_CODE);
- public static final ProcessingException JSON_COMPILATION_ERROR = new ProcessingException(JSON_COMPILATION_ERROR_CODE);
- public static final ProcessingException PQL_PARSING_ERROR = new ProcessingException(PQL_PARSING_ERROR_CODE);
- public static final ProcessingException ACCESS_DENIED_ERROR = new ProcessingException(ACCESS_DENIED_ERROR_CODE);
+ public static final ProcessingException JSON_PARSING_ERROR = new ProcessingException(
Review comment:
Please check the code format setting. Seems you are using 80 characters per line. Pinot Style uses 120 characters instead
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
##########
@@ -167,10 +167,7 @@ public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService e
// TODO: Change broker to watch both IdealState and ExternalView to not query the removed segments
Review comment:
This `TODO` is already done. You may remove this comment block
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
##########
@@ -63,19 +63,27 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTrace) {
public static final int COMBINE_GROUP_BY_EXCEPTION_ERROR_CODE = 600;
public static final int QUERY_VALIDATION_ERROR_CODE = 700;
public static final int UNKNOWN_ERROR_CODE = 1000;
+ public static final int SEGMENTS_NOT_ONLINE_ERROR_CODE = 1001;
+ public static final int SEGMENTS_NOT_ACQUIRED_ERROR_CODE = 1002;
+ public static final int SERVERS_NO_RESPONSE_ERROR_CODE = 1003;
Review comment:
```suggestion
public static final int SERVER_NOT_RESPONDING_ERROR_CODE = 270;
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
##########
@@ -251,6 +248,14 @@ public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService e
.put(MetadataKey.MIN_CONSUMING_FRESHNESS_TIME_MS.getName(), Long.toString(minConsumingFreshnessTimeMs));
}
+ if (numSegmentsQueried > numSegmentsAcquired) {
+ String errorMessage =
+ String.format("Some segments could not be acquired: %d", numSegmentsQueried - numSegmentsAcquired);
Review comment:
Collect the segment names unable to be acquired and put them into the error message
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
##########
@@ -63,19 +63,27 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTrace) {
public static final int COMBINE_GROUP_BY_EXCEPTION_ERROR_CODE = 600;
public static final int QUERY_VALIDATION_ERROR_CODE = 700;
public static final int UNKNOWN_ERROR_CODE = 1000;
+ public static final int SEGMENTS_NOT_ONLINE_ERROR_CODE = 1001;
+ public static final int SEGMENTS_NOT_ACQUIRED_ERROR_CODE = 1002;
Review comment:
```suggestion
public static final int SERVER_SEGMENT_MISSING_ERROR_CODE = 235;
```
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java
##########
@@ -126,6 +126,9 @@ protected BrokerResponseNative processBrokerRequest(long requestId, BrokerReques
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.BROKER_RESPONSES_WITH_PROCESSING_EXCEPTIONS, 1);
}
if (numServersQueried > numServersResponded) {
+ String errorMessage = String.format("Some servers did not respond: %d", numServersQueried - numServersResponded);
Review comment:
Let's collect the missing server name from the `ServerRoutingInstance`
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -794,6 +795,11 @@ private BrokerResponseNative handlePQLRequest(long requestId, String query, Json
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1);
}
+ if (0 != numUnavailableSegments) {
Review comment:
Let's collect the unavailable segment names and put them into the error message
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
##########
@@ -63,19 +63,27 @@ public static void setMaxLinesOfStackTrace(int maxLinesOfStackTrace) {
public static final int COMBINE_GROUP_BY_EXCEPTION_ERROR_CODE = 600;
public static final int QUERY_VALIDATION_ERROR_CODE = 700;
public static final int UNKNOWN_ERROR_CODE = 1000;
+ public static final int SEGMENTS_NOT_ONLINE_ERROR_CODE = 1001;
Review comment:
```suggestion
public static final int BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE = 310;
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org