goingforstudying-ctrl commented on code in PR #2562:
URL:
https://github.com/apache/datafusion-ballista/pull/2562#discussion_r4270613310
##########
ballista/executor/src/runtime_cache.rs:
##########
@@ -80,13 +88,21 @@ pub trait SessionRuntimeCache: Send + Sync {
/// the shared base, which installs a fresh per-task memory pool — so memory
/// isolation is unchanged.
///
+/// The cache key pairs the session id with a fingerprint of the session
+/// config's extension entries (see `config_fingerprint`). The base producer
+/// captures config state when it builds the env — the S3-aware producer
+/// clones the session's `S3Options` into the object-store registry — so a
+/// `SET` that lands between two tasks of the same session must miss the
+/// cache and build a fresh base; otherwise the new task keeps reading with
+/// the settings the session's first task carried.
+///
/// The cache is bounded by an LRU of `capacity` sessions. A capacity of `0`
Review Comment:
Updated in 4ac7497f. All four spots now say the LRU counts (session, config
fingerprint) entries, not sessions: the field doc here,
`DefaultSessionRuntimeCache::new`, the `--session-runtime-cache-capacity` help
text, and the `ExecutorProcessConfig` field doc. I kept the existing eviction
behavior and documented the trade-off instead: entries from a session's earlier
fingerprints stay until LRU eviction, so a reverted SET still hits the original
base.
##########
ballista/executor/src/runtime_cache.rs:
##########
@@ -138,10 +155,52 @@ impl SessionRuntimeCache for DefaultSessionRuntimeCache {
}
}
+/// Fingerprint of the session config's extension entries.
+///
+/// A base [`RuntimeEnv`] bakes in whatever config the producer reads at build
+/// time (e.g. the `S3Options` cloned into the object-store registry), and
+/// every extension entry flows through `SET`-driven task properties, so this
+/// is the part of the config that can legitimately change mid-session.
+/// Built-in `datafusion.*` options are not included: they do not feed the
+/// base env's shared state.
+///
+/// The `ballista` extension is skipped for the same reason: none of the
+/// shipped producers read it, and the client re-sends `ballista.job.name`
+/// with every job, so keying on it would give a session that `SET`s a job
+/// name a fresh fingerprint per query — each one building a new base (and
+/// dropping the footer cache and cross-query reuse) and taking another LRU
+/// slot.
+///
+/// Extension iteration is ordered by prefix, and each extension's entries
+/// are sorted by key, so equal configs always produce equal fingerprints.
+fn config_fingerprint(config: &SessionConfig) -> u64 {
Review Comment:
Done in 4ac7497f. The `RuntimeProducer` and `SessionRuntimeCache` docs now
state the key explicitly: (session id, fingerprint of extension entries
excluding the `ballista` extension). `datafusion.*` and `ballista.*` don't feed
the key, so a producer reading those stays pinned to the first task's values
for the session.
--
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]
