Copilot commented on code in PR #17713:
URL: https://github.com/apache/pinot/pull/17713#discussion_r2815085538
##########
pinot-plugins/pinot-file-system/pinot-gcs/src/main/java/org/apache/pinot/plugin/filesystem/GcsPinotFS.java:
##########
@@ -153,8 +153,11 @@ public boolean deleteBatch(List<URI> segmentUris, boolean
forceDelete)
if (existsDirectoryOrBucket(gcsUri)) {
result &= delete(gcsUri, forceDelete);
} else {
- blobIds.add(getBlob(gcsUri).getBlobId());
- if (blobIds.size() >= DELETE_BATCH_LIMIT || !iterator.hasNext()) {
+ Blob blob = getBlob(gcsUri);
+ if (blob != null) {
+ blobIds.add(blob.getBlobId());
+ }
+ if (blobIds.size() >= DELETE_BATCH_LIMIT || (!blobIds.isEmpty() &&
!iterator.hasNext())) {
Review Comment:
This change adds new behavior for missing blobs (skipping `Storage.get(...)
== null`) and for avoiding `_storage.delete(...)` on an empty batch. There
currently aren’t any tests in the GCS module covering `deleteBatch()` (unlike
e.g. S3/HDFS implementations), so it’s easy for regressions like NPEs or missed
flushes to slip back in. Please add a focused test (ideally with a mocked
`Storage`) to cover null blobs and end-of-iteration batch flushing.
--
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]
