Copilot commented on code in PR #17512:
URL: https://github.com/apache/pinot/pull/17512#discussion_r2739872390
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -664,9 +672,16 @@ private SuccessResponse uploadSegments(String tableName,
TableType tableType, Fo
allowRefresh, headers, segmentUploadMetadataList);
}
}
+ } catch (WebApplicationException e) {
+ throw e;
Review Comment:
Catching and re-throwing WebApplicationException without any processing is
redundant. This catch block can be removed since WebApplicationException will
be caught by the generic Exception handler below or propagate naturally.
```suggestion
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -664,9 +672,16 @@ private SuccessResponse uploadSegments(String tableName,
TableType tableType, Fo
allowRefresh, headers, segmentUploadMetadataList);
}
}
+ } catch (WebApplicationException e) {
+ throw e;
} catch (Exception e) {
-
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SEGMENT_UPLOAD_ERROR,
- segmentUploadMetadataList.size());
+ // Check if the original cause was a quota failure
+ if (e instanceof ControllerApplicationException) {
+
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SEGMENT_UPLOAD_ERROR,
+ segmentUploadMetadataList.size());
+ _controllerMetrics.addMeteredTableValue(tableName,
ControllerMeter.CONTROLLER_TABLE_SEGMENT_UPLOAD_ERROR,
+ segmentUploadMetadataList.size());
Review Comment:
The comment states 'Check if the original cause was a quota failure', but
the code checks if the exception is a ControllerApplicationException, which is
not specific to quota failures. The quota check at line 634 throws a
ControllerApplicationException on failure, but so do other error conditions.
This logic will incorrectly increment quota-related metrics for non-quota
failures. Either update the comment to be accurate, or add more specific
exception handling to distinguish quota failures from other
ControllerApplicationExceptions.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -664,9 +672,16 @@ private SuccessResponse uploadSegments(String tableName,
TableType tableType, Fo
allowRefresh, headers, segmentUploadMetadataList);
}
}
+ } catch (WebApplicationException e) {
+ throw e;
} catch (Exception e) {
-
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SEGMENT_UPLOAD_ERROR,
- segmentUploadMetadataList.size());
+ // Check if the original cause was a quota failure
+ if (e instanceof ControllerApplicationException) {
+
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SEGMENT_UPLOAD_ERROR,
+ segmentUploadMetadataList.size());
+ _controllerMetrics.addMeteredTableValue(tableName,
ControllerMeter.CONTROLLER_TABLE_SEGMENT_UPLOAD_ERROR,
+ segmentUploadMetadataList.size());
+ }
Review Comment:
The metrics are only incremented when the caught exception is already a
ControllerApplicationException, but then a new ControllerApplicationException
is thrown wrapping the original. This means that if a
ControllerApplicationException is thrown from the quota check (line 634), the
metrics will be incremented correctly. However, for all other exceptions that
are NOT ControllerApplicationException, the metrics will not be incremented
before wrapping them. This creates inconsistent metric tracking where some
upload errors are counted and others are not.
```suggestion
_controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SEGMENT_UPLOAD_ERROR,
segmentUploadMetadataList.size());
_controllerMetrics.addMeteredTableValue(tableName,
ControllerMeter.CONTROLLER_TABLE_SEGMENT_UPLOAD_ERROR,
segmentUploadMetadataList.size());
```
--
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]
