goingforstudying-ctrl commented on code in PR #2562:
URL:
https://github.com/apache/datafusion-ballista/pull/2562#discussion_r4252361238
##########
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 all four spots: the struct doc, `DefaultSessionRuntimeCache::new`,
the `--session-runtime-cache-capacity` help text in `config.rs`, and the field
doc in `executor_process.rs`. They now say the LRU counts (session,
fingerprint) entries, so a session changing its extension config mid-session
occupies one entry per distinct fingerprint.
Kept the current stale-entry behavior: reverting a SET back to the original
values hits the warm base (pinned by
`reverted_s3_options_hit_original_base_runtime`), and the SET / query /
SET-back pattern is worth keeping fast. The struct doc now spells out the
trade-off.
##########
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. Both `RuntimeProducer` and `SessionRuntimeCache` now document the key:
(session id, fingerprint of extension entries minus the `ballista` extension).
They also call out that a producer reading `datafusion.*` or `ballista.*`
options at build time keeps the values seen by the first task for the rest of
the session. Only extension options such as `s3.*` rekey the cache.
--
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]
