cryptoe commented on code in PR #13198:
URL: https://github.com/apache/druid/pull/13198#discussion_r991329095
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java:
##########
@@ -851,19 +858,28 @@ public void verifyResults()
Preconditions.checkArgument(expectedDataSource != null, "dataSource
cannot be null");
Preconditions.checkArgument(expectedRowSignature != null,
"expectedRowSignature cannot be null");
Preconditions.checkArgument(
- expectedResultRows != null || expectedMSQFault != null,
- "atleast one of expectedResultRows or expectedMSQFault should be set
to non null"
+ expectedResultRows != null || expectedMSQFault != null ||
expectedMSQFaultClass != null,
+ "atleast one of expectedResultRows, expectedMSQFault or
expectedMSQFaultClass should be set to non null"
);
Preconditions.checkArgument(expectedShardSpec != null, "shardSpecClass
cannot be null");
readyToRun();
try {
String controllerId = runMultiStageQuery(sql, queryContext);
- if (expectedMSQFault != null) {
+ if (expectedMSQFault != null || expectedMSQFaultClass != null) {
MSQErrorReport msqErrorReport = getErrorReportOrThrow(controllerId);
- Assert.assertEquals(
- expectedMSQFault.getCodeWithMessage(),
- msqErrorReport.getFault().getCodeWithMessage()
- );
+ if (expectedMSQFault != null) {
+ Assert.assertEquals(
+ expectedMSQFault.getCodeWithMessage(),
+ msqErrorReport.getFault().getCodeWithMessage()
+ );
+ }
+ if (expectedMSQFaultClass != null) {
Review Comment:
Shouldn't we always assert the MSQ fault?
Is there any specific reason to assert on the MSQ class?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -642,12 +642,22 @@ public void updateCounters(CounterSnapshotsTree
snapshotsTree)
// Present means the warning limit was exceeded, and warnings have
therefore turned into an error.
String errorCode = warningsExceeded.get().lhs;
Long limit = warningsExceeded.get().rhs;
- workerError(MSQErrorReport.fromFault(
- id(),
- selfDruidNode.getHost(),
- null,
- new TooManyWarningsFault(limit.intValue(), errorCode)
- ));
+
+ if (limit == 0L) {
+ for (MSQErrorReport errorReport1 : workerWarnings) {
Review Comment:
nit: I think we should rename the variable to warningReport.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -642,12 +642,22 @@ public void updateCounters(CounterSnapshotsTree
snapshotsTree)
// Present means the warning limit was exceeded, and warnings have
therefore turned into an error.
String errorCode = warningsExceeded.get().lhs;
Long limit = warningsExceeded.get().rhs;
- workerError(MSQErrorReport.fromFault(
- id(),
- selfDruidNode.getHost(),
- null,
- new TooManyWarningsFault(limit.intValue(), errorCode)
- ));
+
+ if (limit == 0L) {
+ for (MSQErrorReport errorReport1 : workerWarnings) {
+ if
(errorReport1.getFault().getErrorCode().equalsIgnoreCase(errorCode)) {
+ workerError(errorReport1);
+ break;
+ }
+ }
Review Comment:
We should also throw an illegal state exception if we donot find the error
code in the workerWarnings.
--
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]
