andygrove commented on code in PR #2562:
URL:
https://github.com/apache/datafusion-ballista/pull/2562#discussion_r4221892744
##########
ballista/executor/src/runtime_cache.rs:
##########
@@ -138,10 +153,41 @@ 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.
+///
+/// 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 {
+ let mut hasher = DefaultHasher::new();
+ for (prefix, extension) in config.options().extensions.iter() {
Review Comment:
Should we skip the `ballista` extension here too, the same way
`datafusion.*` is left out? None of the shipped producers read `ballista.*`,
but the client sends `ballista.job.name` with every job. So a session that does
`SET ballista.job.name = '...'` before each query gets a new fingerprint per
query. Each of those builds a fresh base env, which drops the session's footer
cache and the cross-query reuse from #1995. It also takes another LRU slot each
time. Skipping `BallistaConfig::PREFIX` would still keep `s3.*` and any custom
extensions in the key, and that covers the provider options proposed in #2532
since those live under `s3.*` too. It might be worth adding a test that
changing `ballista.job.name` still hits the cached base.
##########
ballista/executor/src/runtime_cache.rs:
##########
@@ -80,13 +86,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:
Small one. Since the key is now session plus config, `capacity` counts
session and config pairs rather than sessions. A session that rotates its S3
token a few times holds a slot per token until the old ones age out. Could we
update this line, the `new()` docs ("invoked at most once per cached session"),
and the `--session-runtime-cache-capacity` help text in
`executor/src/config.rs` to match?
--
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]
