Copilot commented on code in PR #17586:
URL: https://github.com/apache/pinot/pull/17586#discussion_r2739488282
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/PinotFSSegmentFetcher.java:
##########
@@ -34,4 +43,36 @@ protected void fetchSegmentToLocalWithoutRetry(URI uri, File
dest)
PinotFSFactory.create(uri.getScheme()).copyToLocalFile(uri, dest);
}
}
+
+ @Override
+ public File fetchUntarSegmentToLocalStreamed(URI uri, File dest, long
rateLimit, AtomicInteger attempts)
+ throws Exception {
+ PinotFS pinotFS;
+ if (uri.getScheme() == null) {
+ pinotFS = PinotFSFactory.create(PinotFSFactory.LOCAL_PINOT_FS_SCHEME);
+ } else {
+ pinotFS = PinotFSFactory.create(uri.getScheme());
+ }
+ AtomicReference<File> untarredFileRef = new AtomicReference<>();
+
+ int tries;
+ try {
+ tries =
+ RetryPolicies.exponentialBackoffRetryPolicy(_retryCount,
_retryWaitMs, _retryDelayScaleFactor).attempt(() -> {
+ try (InputStream inputStream = pinotFS.open(uri)) {
+ List<File> untarredFiles =
TarCompressionUtils.untarWithRateLimiter(inputStream, dest, rateLimit);
+ untarredFileRef.set(untarredFiles.get(0));
+ return true;
+ } catch (Exception e) {
+ _logger.warn("Caught exception while downloading segment from:
{} to: {}", uri, dest, e);
+ return false;
+ }
+ });
+ } catch (AttemptsExceededException | RetriableOperationException e) {
+ attempts.set(e.getAttempts());
+ throw e;
+ }
+ attempts.set(tries);
+ return untarredFileRef.get();
Review Comment:
If all retry attempts fail but no exception is thrown, untarredFileRef.get()
could return null. This would cause issues for callers expecting a valid File.
Consider throwing an exception if untarredFileRef is null after retries
complete.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/PinotFSSegmentFetcher.java:
##########
@@ -34,4 +43,36 @@ protected void fetchSegmentToLocalWithoutRetry(URI uri, File
dest)
PinotFSFactory.create(uri.getScheme()).copyToLocalFile(uri, dest);
}
}
+
+ @Override
+ public File fetchUntarSegmentToLocalStreamed(URI uri, File dest, long
rateLimit, AtomicInteger attempts)
+ throws Exception {
+ PinotFS pinotFS;
+ if (uri.getScheme() == null) {
+ pinotFS = PinotFSFactory.create(PinotFSFactory.LOCAL_PINOT_FS_SCHEME);
+ } else {
+ pinotFS = PinotFSFactory.create(uri.getScheme());
+ }
+ AtomicReference<File> untarredFileRef = new AtomicReference<>();
+
+ int tries;
+ try {
+ tries =
+ RetryPolicies.exponentialBackoffRetryPolicy(_retryCount,
_retryWaitMs, _retryDelayScaleFactor).attempt(() -> {
+ try (InputStream inputStream = pinotFS.open(uri)) {
+ List<File> untarredFiles =
TarCompressionUtils.untarWithRateLimiter(inputStream, dest, rateLimit);
Review Comment:
Direct access to index 0 without checking if the list is empty could cause
IndexOutOfBoundsException. Add a check to verify that untarredFiles is not
empty before accessing the first element.
```suggestion
List<File> untarredFiles =
TarCompressionUtils.untarWithRateLimiter(inputStream, dest, rateLimit);
if (untarredFiles.isEmpty()) {
_logger.warn("No files found after untarring segment from:
{} to: {}", uri, dest);
return false;
}
```
--
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]
