Copilot commented on code in PR #16553:
URL: https://github.com/apache/pinot/pull/16553#discussion_r2263866318
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -702,6 +740,28 @@ public void copyToLocalFile(URI srcUri, File dstFile)
_s3Client.getObject(getObjectRequest, ResponseTransformer.toFile(dstFile));
}
+ @Override
+ public void copyToLocalFile(URI srcUri, File dstFile)
+ throws Exception {
+ try {
+ LOGGER.info("Copy {} to local {}", srcUri, dstFile.getAbsolutePath());
+ copyToLocalFileInternal(srcUri, dstFile);
+ } catch (Exception e) {
+ LOGGER.warn("Caught exception during S3 copy, attempting to refresh
credentials and retry");
+
+ logAwsCredentials("BEFORE refresh");
+ initOrRefreshS3Client();
+ logAwsCredentials("AFTER refresh");
+
+ try {
+ copyToLocalFileInternal(srcUri, dstFile);
+ } catch (Exception retryException) {
+ throw new Exception("Unexpected exception during S3 copy to local file
after credential refresh",
+ retryException);
+ }
Review Comment:
The retry logic catches all exceptions and assumes they are
credential-related. Consider checking the exception type to ensure credential
refresh is only attempted for authentication/authorization failures, not for
network issues or other unrelated errors.
```suggestion
// Only attempt credential refresh and retry for S3Exception with
401/403 status code
if (e instanceof S3Exception) {
int statusCode = ((S3Exception) e).statusCode();
if (statusCode == 401 || statusCode == 403) {
LOGGER.warn("Caught S3 authentication/authorization exception
({}), attempting to refresh credentials and retry", statusCode);
logAwsCredentials("BEFORE refresh");
initOrRefreshS3Client();
logAwsCredentials("AFTER refresh");
try {
copyToLocalFileInternal(srcUri, dstFile);
} catch (Exception retryException) {
throw new Exception("Unexpected exception during S3 copy to
local file after credential refresh",
retryException);
}
return;
}
}
// For all other exceptions, rethrow
throw e;
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -690,10 +696,42 @@ private void visitFiles(URI fileUri, boolean recursive,
Consumer<S3Object> objec
}
}
- @Override
- public void copyToLocalFile(URI srcUri, File dstFile)
+ private static String maskKey(String key) {
+ if (key == null || key.length() <= 6) {
+ return "***";
+ }
+ int maskLength = key.length() - 6;
+ StringBuilder sb = new StringBuilder();
+ sb.append(key, 0, 3);
+ for (int i = 0; i < maskLength; i++) {
+ sb.append("*");
+ }
+ sb.append(key, key.length() - 3, key.length());
+ return sb.toString();
+ }
+
+ private void logAwsCredentials(String when) {
+ AwsCredentials credentials = getAwsCredentials();
+ if (credentials != null) {
+ LOGGER.warn("S3 credentials {} - Access Key: {}", when,
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials {} - Secret Key: {}", when,
maskKey(credentials.secretAccessKey()));
+ } else {
+ LOGGER.warn("S3 credentials {} - Unable to retrieve AWS credentials
(access key & secret key unavailable)", when);
Review Comment:
Logging AWS credentials (even masked) at WARN level could expose sensitive
information in logs. Consider using DEBUG level or removing credential logging
entirely for production environments.
```suggestion
LOGGER.debug("S3 credentials {} - Access Key: {}", when,
maskKey(credentials.accessKeyId()));
LOGGER.debug("S3 credentials {} - Secret Key: {}", when,
maskKey(credentials.secretAccessKey()));
} else {
LOGGER.debug("S3 credentials {} - Unable to retrieve AWS credentials
(access key & secret key unavailable)", when);
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -690,10 +696,42 @@ private void visitFiles(URI fileUri, boolean recursive,
Consumer<S3Object> objec
}
}
- @Override
- public void copyToLocalFile(URI srcUri, File dstFile)
+ private static String maskKey(String key) {
+ if (key == null || key.length() <= 6) {
+ return "***";
+ }
+ int maskLength = key.length() - 6;
+ StringBuilder sb = new StringBuilder();
+ sb.append(key, 0, 3);
+ for (int i = 0; i < maskLength; i++) {
+ sb.append("*");
+ }
+ sb.append(key, key.length() - 3, key.length());
+ return sb.toString();
+ }
+
+ private void logAwsCredentials(String when) {
+ AwsCredentials credentials = getAwsCredentials();
+ if (credentials != null) {
+ LOGGER.warn("S3 credentials {} - Access Key: {}", when,
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials {} - Secret Key: {}", when,
maskKey(credentials.secretAccessKey()));
+ } else {
+ LOGGER.warn("S3 credentials {} - Unable to retrieve AWS credentials
(access key & secret key unavailable)", when);
+ }
+ }
+
+ public AwsCredentials getAwsCredentials() {
+ Object provider =
_s3Client.serviceClientConfiguration().credentialsProvider();
+ if (provider instanceof AwsCredentialsProvider) {
+ return ((AwsCredentialsProvider) provider).resolveCredentials();
+ } else {
+ throw new IllegalStateException("S3 client credentialsProvider is not an
AwsCredentialsProvider: "
+ + (provider != null ? provider.getClass().getName() : "null"));
Review Comment:
The getAwsCredentials method throws an IllegalStateException when the
credentials provider is not the expected type. This could cause the credential
refresh to fail unexpectedly. Consider returning null or handling this case
more gracefully to avoid breaking the retry mechanism.
```suggestion
LOGGER.warn("S3 client credentialsProvider is not an
AwsCredentialsProvider: {}",
(provider != null ? provider.getClass().getName() : "null"));
return null;
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -702,6 +740,28 @@ public void copyToLocalFile(URI srcUri, File dstFile)
_s3Client.getObject(getObjectRequest, ResponseTransformer.toFile(dstFile));
}
+ @Override
+ public void copyToLocalFile(URI srcUri, File dstFile)
+ throws Exception {
+ try {
+ LOGGER.info("Copy {} to local {}", srcUri, dstFile.getAbsolutePath());
+ copyToLocalFileInternal(srcUri, dstFile);
+ } catch (Exception e) {
+ LOGGER.warn("Caught exception during S3 copy, attempting to refresh
credentials and retry");
+
+ logAwsCredentials("BEFORE refresh");
+ initOrRefreshS3Client();
+ logAwsCredentials("AFTER refresh");
+
+ try {
+ copyToLocalFileInternal(srcUri, dstFile);
+ } catch (Exception retryException) {
+ throw new Exception("Unexpected exception during S3 copy to local file
after credential refresh",
Review Comment:
The error message 'Unexpected exception during S3 copy to local file after
credential refresh' is generic and doesn't provide context about the original
failure. Include information about both the original exception and the retry
exception to aid in debugging.
```suggestion
throw new Exception(
"Unexpected exception during S3 copy to local file after
credential refresh. "
+ "Original exception: " + e.getClass().getName() + ": " +
e.getMessage() + "; "
+ "Retry exception: " + retryException.getClass().getName()
+ ": " + retryException.getMessage(),
```
--
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]
