AWS: Prevent excessive creation of auth sessions in S3V4RestSignerClient#13215
Conversation
d6e7493 to
f72358f
Compare
| return authSession; | ||
| } | ||
|
|
||
| static { |
There was a problem hiding this comment.
can we move this further to the top?
|
Heads up: I need more changes to make this work properly. |
f72358f to
58342c0
Compare
Done, this is now ready for review. FYI we needed to implement session caching for sessions obtained via the |
| sessionCache = newSessionCache(name, properties); | ||
| } | ||
|
|
||
| if (config.token() != null) { |
There was a problem hiding this comment.
This is basically the same logic as in catalogSession(), but with caching enabled.
| @VisibleForTesting | ||
| AuthSession authSession() { | ||
| if (null == authSession) { | ||
| private AuthManager authManager() { |
There was a problem hiding this comment.
could you please move the authManager() method just below authSession() as it's causing a diff that adds visual overhead during review
See apache/iceberg#13215 for context. In Iceberg 1.9.0, "standalone" sessions created by the S3 request signer client weren't cached, and an AuthManager was created for each signer instance. This is a performance degradation compared to the situation pre-AuthManager, where sessions were cached by the signer itself, and the cache was a static field. In Iceberg 1.10.0, this should be fixed, but also requires this AuthManager to implement proper caching of sessions created by the signer client.
See apache/iceberg#13215 for context. In Iceberg 1.9.0, "standalone" sessions created by the S3 request signer client weren't cached, and an AuthManager was created for each signer instance. This is a performance degradation compared to the situation pre-AuthManager, where sessions were cached by the signer itself, and the cache was a static field. In Iceberg 1.10.0, this should be fixed, but also requires this AuthManager to implement proper caching of sessions created by the signer client.
See apache/iceberg#13215 for context. In Iceberg 1.9.0, "standalone" sessions created by the S3 request signer client weren't cached, and an AuthManager was created for each signer instance. This is a performance degradation compared to the situation pre-AuthManager, where sessions were cached by the signer itself, and the cache was a static field. In Iceberg 1.10.0, this should be fixed, but also requires this AuthManager to implement proper caching of sessions created by the signer client.
|
|
||
| @Override | ||
| public AuthSession tableSession(RESTClient sharedClient, Map<String, String> properties) { | ||
|
|
nastra
left a comment
There was a problem hiding this comment.
just a few minor comments that would be good to address but overall the approach LGTM. @danielcweeks could you also take a look please?
| @SuppressWarnings("immutables:incompat") | ||
| private volatile AuthSession authSession; | ||
| @SuppressWarnings("ShutdownHook") | ||
| private static void installShutdownHook() { |
There was a problem hiding this comment.
Are we sure this is safe? It feels like we could be trading one problem for another because if an engine loads tasks in separate classloaders, we might have an issue with shutdown hooks piling up.
There was a problem hiding this comment.
Fair point. I added the hook, it wasn't there before. The HTTP client for example wasn't being closed before.
I guess it's OK to remove the hook. (I can also look into adding phantom references here, but I'd prefer to do that in a follow-up task, since this PR is kind of urgent.)
There was a problem hiding this comment.
I don't think we're making the situation here any worse and I find shutdown hooks not a great solution because we're adding complexity and still not shutting them down based on when they're no longer necessary, but rather when you're already exiting the jvm.
Since Iceberg 1.10 it is no longer possible to use different auth configurations with S3 remote signing. This is caused by apache/iceberg#13215 which keeps auth related configuration in static fields in `o.a.i.aws.s3.signer.S3V4RestSignerClient`.
Since Iceberg 1.10 it is no longer possible to use different auth configurations with S3 remote signing. This is caused by apache/iceberg#13215 which keeps auth related configuration in static fields in `o.a.i.aws.s3.signer.S3V4RestSignerClient`.
Since Iceberg 1.10 it is no longer possible to use different auth configurations with S3 remote signing. This is caused by apache/iceberg#13215 which keeps auth related configuration in static fields in `o.a.i.aws.s3.signer.S3V4RestSignerClient`.
…sionCache (#242) The double-checked locking pattern in getOrCreateSessionCache() had a bug: the local variable 'cache' was not updated after another thread won the initialization race inside the synchronized block. The losing thread would return null, causing a NullPointerException at cache.cachedSession() in tableSession() (line 94). This surfaces when multiple threads call tableSession() simultaneously on the first Iceberg commit — for example, when an Iceberg Kafka Connect sink flushes several Parquet files in parallel. It became observable in iceberg-kafka-connect 1.11.0 after apache/iceberg#13215 changed S3V4RestSignerClient to call authManager().tableSession() on every S3 request instead of caching the session per instance. Fix: re-read sessionCache from the volatile field inside the synchronized block so the losing thread picks up the value set by the winner.
This PR fixes an oversight in
S3V4RestSignerClientintroduced by the switch to the AuthManager API: the session cache was previously a static field, but became an instance field. As a result, too many token fetches are being observed when using request signing.