BootstrapperSBL opened a new pull request, #16109: URL: https://github.com/apache/iceberg/pull/16109
Closes #16034. `S3FileIO.storageCredentials` and `GCSFileIO.storageCredentials` are declared `volatile` but back the reference with a plain `ArrayList`. The reference is read from `clientByPrefix()` / `storageByPrefix()` outside of the synchronized block that publishes the client map, and is replaced wholesale during a scheduled credential refresh. `volatile` covers visibility of the reference, but it does not cover the list contents if the backing `ArrayList` is ever modified in place. Readers iterating outside synchronization rely on the refresh path publishing a stable snapshot. This switches the backing list to `CopyOnWriteArrayList`, which gives readers a consistent snapshot view even when the reference is replaced or the list is mutated, and is still compatible with the Kryo round-trip serialization that the existing FileIO tests exercise. ### Before ```java // use modifiable collection for Kryo serde private volatile List<StorageCredential> storageCredentials = Lists.newArrayList(); ``` ### After ```java // CopyOnWriteArrayList gives a safe snapshot view for readers iterating outside // the synchronized block in clientByPrefix() and is Kryo-serialisable private volatile List<StorageCredential> storageCredentials = new CopyOnWriteArrayList<>(); ``` I considered the alternatives called out in the issue (`List.copyOf`, `Collections.unmodifiableList`, `ImmutableList.copyOf`) but Kryo's default `CollectionSerializer` rebuilds collections via `add()` during deserialization, which throws `UnsupportedOperationException` on any of those wrapper types and broke the existing `fileIOWithStorageCredentialsSerialization` / `resolvingFileIOLoadWithStorageCredentials` tests. `CopyOnWriteArrayList` supports `add()` and round-trips cleanly. ### Tests - Added `setCredentialsPublishesThreadSafeSnapshot` in `TestS3FileIOCredentialRefresh` and `TestGCSFileIO` to verify the field type after `setCredentials`. - Re-ran `./gradlew :iceberg-aws:test --tests "*S3FileIO*"` and `./gradlew :iceberg-gcp:test --tests "*GCSFileIO*"`; both green, including the Kryo / Java serialization round trips. - `./gradlew :iceberg-aws:spotlessCheck :iceberg-gcp:spotlessCheck` passes. -- 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]
