obelix74 commented on code in PR #3414:
URL: https://github.com/apache/polaris/pull/3414#discussion_r2683510303
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -60,10 +60,38 @@ public interface StorageCredentialCacheKey {
* The credential vending context for session tags. When session tags are
enabled, this contains
* the catalog, namespace, table, and roles information. When session tags
are disabled, this
* should be {@link CredentialVendingContext#empty()} to ensure consistent
cache key behavior.
+ *
+ * <p>Note: The trace ID in the context is marked as {@code
@Value.Auxiliary} and is therefore
+ * excluded from equals/hashCode. See {@link #traceIdForCaching()} for how
trace IDs are handled
+ * in cache key comparison.
*/
@Value.Parameter(order = 9)
CredentialVendingContext credentialVendingContext();
+ /**
+ * The trace ID to include in cache key comparison. This is separate from
the trace ID in {@link
+ * #credentialVendingContext()} because:
+ *
+ * <ul>
+ * <li>The context's trace ID is marked as {@code @Value.Auxiliary} and is
excluded from
+ * equals/hashCode
+ * <li>When {@code INCLUDE_TRACE_ID_IN_SESSION_TAGS} is disabled
(default), trace IDs don't
+ * affect credentials, so this should be {@code Optional.empty()} for
cache efficiency
+ * <li>When {@code INCLUDE_TRACE_ID_IN_SESSION_TAGS} is enabled, trace IDs
DO affect credentials
+ * (via session tags), so this should contain the trace ID for cache
correctness
+ * </ul>
+ *
+ * <p>This explicit field ensures cache correctness: credentials with
different trace IDs are
+ * correctly treated as different cache entries when (and only when) trace
IDs affect the
+ * credentials.
+ */
+ @Value.Parameter(order = 10)
+ Optional<String> traceIdForCaching();
Review Comment:
I implemented this by making CredentialVendingContext.traceId() a normal
property (removed @Value.Auxiliary), so it naturally participates in
equality/hashCode when present. With that, I removed the separate
traceIdForCaching field from StorageCredentialCacheKey entirely. It was
related to your third observation.
--
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]
