xiangfu0 commented on code in PR #18644:
URL: https://github.com/apache/pinot/pull/18644#discussion_r3334003089
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -316,10 +316,14 @@ public void init(S3Client s3Client) {
*/
public void init(S3Client s3Client, String serverSideEncryption,
PinotConfiguration serverSideEncryptionConfig) {
_s3Client = s3Client;
- S3Config s3Config = new S3Config(serverSideEncryptionConfig);
- setServerSideEncryption(serverSideEncryption, s3Config);
- setMultiPartUploadConfigs(s3Config);
- setDisableAcl(s3Config);
+ // Store the config on the _s3Config field rather than a local. A later
credential refresh goes
+ // through initOrRefreshS3Client(), which reads _s3Config.getRegion();
leaving the field null
+ // caused a NullPointerException on the first refresh for instances
initialized with a provided
+ // client.
+ _s3Config = new S3Config(serverSideEncryptionConfig);
Review Comment:
This fixes the `init(S3Client, ..., PinotConfiguration)` path, but the other
public provided-client overload at `init(S3Client)` still leaves `_s3Config`
null. If a caller uses that overload and later hits the 401/403 retry path,
`initOrRefreshS3Client()` will still dereference `_s3Config.getRegion()` and
fail before the retry happens. Please either initialize `_s3Config` there as
well or explicitly guard/disable credential refresh for bare-client
initialization.
--
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]
