Re: [PR] [LIVY-1079] Spark on k8s : changes for using supporting multi tenant(multi namespace) k8s cluster [livy]

["gyogal (via GitHub)" <gi...@apache.org>]

gyogal commented on PR #462:
URL: https://github.com/apache/livy/pull/462#issuecomment-5701912820

   Thanks @ashokkumarrathore ! I asked Claude to review the PR and here are the main potential issues found. Can you please check if these are relevant?
   
     **LIVY-461 — a few things worth addressing before merge:**
   
     1. **Namespace resolution ignores `SPARK_CONF_DIR`** (High). `namespaceFromSparkDefaults` only reads `$SPARK_HOME/conf/spark-defaults.conf`, but Spark resolves
     `SPARK_CONF_DIR` first. If the namespace is set there, it's missed → falls back to `"default"` → the namespace-scoped LIST never finds the driver. (The old `inAnyNamespace` behavior masked this.)
   
     2. **Dead `.inNamespace(namespace)` wrappers** (Medium). Every extension method already scopes internally via `app.getApplicationNamespace`, so the
     `kubernetesClient.inNamespace(namespace).<ext>(...)` call sites (and the leaked-GC one) are no-ops. Drop them, and scope `killApplication`'s delete explicitly to the pod's namespace for namespaced-RBAC teardown.
   
     3. **Recovered pre-upgrade session pins `""` into `appNamespaces`** (Medium). A session recovered from pre-LIVY-461 metadata has `namespace = ""`, which gets added to
     the GC set and never removed → every GC cycle does a cluster-wide `inAnyNamespace` scan, forbidden for a namespace-scoped service account. Guard with `if (namespace.nonEmpty)`.
   
     4. **Rollback breaks recovery deserialization** (Low). The recovery `ObjectMapper` doesn't disable `FAIL_ON_UNKNOWN_PROPERTIES`, so rolling back to a pre-LIVY-461 build
     throws on the new `namespace` field. Add `.configure(FAIL_ON_UNKNOWN_PROPERTIES, false)`.
   
     5. **fabric8 5.6.0 → 6.8.1 is a breaking major bump** (High). Needs the 6.x API migration: `KubernetesClientBuilder` instead of `DefaultKubernetesClient`, `delete()`
     now returns `List<StatusDetails>` (not `boolean`), and `KubernetesClient` types in place of `DefaultKubernetesClient`. Please confirm this was intentional and migrated, not just a version pin.


-- 
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: commits-unsubscribe@livy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org