Copilot commented on code in PR #17512:
URL: https://github.com/apache/pinot/pull/17512#discussion_r2739568937
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -627,8 +627,14 @@ private SuccessResponse uploadSegments(String tableName,
TableType tableType, Fo
SegmentValidationUtils.validateTimeInterval(segmentMetadata,
tableConfig);
}
// TODO: Include the un-tarred segment size when using the METADATA
push rest API. Currently we can only use the
- // tarred segment size as an approximation. Additionally, add the
storage quota check for batch upload mode.
+ // tarred segment size as an approximation.
long segmentSizeInBytes = getSegmentSizeFromFile(sourceDownloadURIStr);
+ if (segmentSizeInBytes < 0) {
+ segmentSizeInBytes = FileUtils.sizeOfDirectory(tempSegmentDir);
+ }
+ // Adding Storage Quota Check
Review Comment:
The comment 'Adding Storage Quota Check' is not descriptive enough. Consider
explaining why the quota check is performed here and what happens when the
check fails (e.g., 'Check storage quota before committing segment; throws
exception if quota exceeded').
```suggestion
// Check table storage quota before committing the segment; aborts
upload (throws) if quota is exceeded.
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -664,9 +670,13 @@ private SuccessResponse uploadSegments(String tableName,
TableType tableType, Fo
allowRefresh, headers, segmentUploadMetadataList);
}
}
+ } catch (WebApplicationException e) {
Review Comment:
The catch block for `WebApplicationException` immediately rethrows the
exception without any additional handling. While this preserves the original
exception type, consider adding a log statement to document that this exception
path was taken, especially since the general `Exception` catch block below logs
metrics. This would aid in debugging and monitoring.
```suggestion
} catch (WebApplicationException e) {
LOGGER.warn("WebApplicationException while uploading segments for
table {}: {}", tableNameWithType,
e.getMessage(), e);
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -627,8 +627,14 @@ private SuccessResponse uploadSegments(String tableName,
TableType tableType, Fo
SegmentValidationUtils.validateTimeInterval(segmentMetadata,
tableConfig);
}
// TODO: Include the un-tarred segment size when using the METADATA
push rest API. Currently we can only use the
- // tarred segment size as an approximation. Additionally, add the
storage quota check for batch upload mode.
+ // tarred segment size as an approximation.
long segmentSizeInBytes = getSegmentSizeFromFile(sourceDownloadURIStr);
+ if (segmentSizeInBytes < 0) {
+ segmentSizeInBytes = FileUtils.sizeOfDirectory(tempSegmentDir);
+ }
+ // Adding Storage Quota Check
+ SegmentValidationUtils.checkStorageQuota(segmentName,
segmentSizeInBytes, segmentSizeInBytes,
+ tableConfig, _storageQuotaChecker);
Review Comment:
The `checkStorageQuota` method is called with `segmentSizeInBytes` twice as
both the second and third arguments. Verify if both parameters should have the
same value or if one should represent a different size metric (e.g.,
uncompressed vs compressed size). If they should differ, ensure the correct
values are passed.
--
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]
