Fix NPE in S3PinotFS credential refresh when initialized with a provided S3Client#18644
Fix NPE in S3PinotFS credential refresh when initialized with a provided S3Client#18644noob-se7en wants to merge 1 commit into
Conversation
…ded S3Client `S3PinotFS.init(S3Client, String, PinotConfiguration)` set `_s3Client` but left the `_s3Config` field null — it built an `S3Config` only as a local for the server-side-encryption / multipart / ACL setup. The single-arg `init(PinotConfiguration)` is the only path that assigned `_s3Config`. Any later operation routed through `retryWithS3CredentialRefresh(...)` -> `initOrRefreshS3Client()` reads `_s3Config.getRegion()`, so an instance created with a caller-provided client threw a NullPointerException on the first credential refresh. Assign the already-constructed `S3Config` to the `_s3Config` field (and reuse it for the SSE/multipart/ACL setup). No extra object construction; the field is now populated so a refresh rebuilds the client from the same config instead of NPEing. Adds `S3PinotFSInitTest` which initializes via the three-arg `init` and then calls `initOrRefreshS3Client()` — it reproduces the NPE before this change and passes after. Static credentials keep the rebuild fully offline (no container). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
noob-se7en
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18644 +/- ##
============================================
+ Coverage 64.27% 64.42% +0.14%
- Complexity 1137 1299 +162
============================================
Files 3314 3362 +48
Lines 204064 207907 +3843
Branches 31760 32463 +703
============================================
+ Hits 131165 133945 +2780
- Misses 62373 63180 +807
- Partials 10526 10782 +256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal issue; see inline comment.
| // 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); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I believe this is unrelated and can be as a followup.
|
closing as duplicate #18679 was merged. |
3 participants
Problem
S3PinotFS.init(S3Client, String, PinotConfiguration)(the "bring your own client" overload) sets_s3Clientbut leaves the_s3Configfieldnull. It constructs anS3Configonly as a local variable, used for the server-side-encryption / multipart / ACL setup. The single-arginit(PinotConfiguration)is currently the only method that assigns the_s3Configfield.Any later operation that goes through the credential-refresh path —
retryWithS3CredentialRefresh(...)→initOrRefreshS3Client()— reads_s3Config.getRegion(). For an instance initialized with a caller-provided client,_s3Configis stillnull, so the first credential refresh throws aNullPointerException.Fix
Assign the
S3Configthat the three-arginitalready constructs to the_s3Configfield (and reuse it for the existing SSE/multipart/ACL calls). No extra object construction. The field is now populated, so a later refresh rebuilds the client from the same config instead of NPEing.Testing
Adds
S3PinotFSInitTest.testInitWithProvidedClientPopulatesConfigForRefresh: initializes via the three-arginit, then callsinitOrRefreshS3Client(). It reproduces theNullPointerExceptionbefore this change and passes after. Static credentials keep the rebuild fully offline (no S3 mock container required).