dongjoon-hyun commented on PR #59277: URL: https://github.com/apache/spark/pull/59277#issuecomment-6048171350
@venkata91, in addition to the review above, I'd like to share some alternative designs that might avoid most of those issues structurally. **Context** The tombstone exists so that clients do not accidentally end up with a fresh, empty server-side session. When SPARK-50710 added `allow_reconnect`, it deliberately chose a per-client opt-in over `closedSessionsTombstonesSize=0`, because the latter would allow every client to reconnect. The gap this PR targets is narrower: when the server closes a session by expiry, the client never had a chance to say `allow_reconnect`. **Alternatives** 1. **Opt in while the session is alive, and let expiry honor it (my preference).** The client declares, for example via a session-scoped conf set through the `Config` RPC, that its session does not depend on server-side state. The flag is captured in `SessionHolderInfo`, and `removeSessionHolder` / `shutdownSessionHolder` check it together with `allowReconnect` when deciding whether to write a tombstone. The decision is made inside the existing close path, so no separate `invalidate` can race with an in-flight shutdown ([1], [2]). Only the session itself can opt in, so nobody can lift another client's fence after the fact ([5]). This extends the SPARK-50710 opt-in model to server-initiated closes and needs no proto change. 2. **Time-based expiry of tombstones.** If the root problem is that tombstones only disappear through size eviction, an `expireAfterWrite` on `closedSessionsCache` (with a new conf) addresses it directly. Since tombstones can already be evicted under load today, this mostly makes the protection window predictable. It does not allow recreating a session immediately, so it complements (1) rather than replacing it. 3. **Keep the after-the-fact release, but verify the owner.** If clearing an existing tombstone is really required, `ReleaseSessionRequest` could carry `client_observed_server_side_session_id` like most other requests. The server would then remove the tombstone only if its `serverSessionId` matches and `closedTimeMs` is defined (i.e. the shutdown has completed), atomically via `closedSessionsCache.asMap.remove(key, info)`. That resolves [1], [2] and [5]. The client side ([3], [4]) still needs to stop using the old session object. 4. **No server change.** Map a stable logical id to a fresh physical session id on the client or gateway side. SPARK-50710 already acknowledged the observability cost of changing session ids, but it is the zero-risk baseline. **A related issue that affects all options** `SessionHolder.close()` cleans up by `SessionKey`, through `SparkConnectExecutionManager.removeAllExecutionsForSession(key)` and `SparkConnectStreamingQueryCache.cleanupRunningQueries` (matching `userId` / `sessionId`). Any path that lets the same key be recreated before `close()` finishes can therefore tear down the new session. This already exists for the SPARK-50710 path on an active session: the holder is removed from the store without a tombstone and then closed, so a concurrent request with the same key can create a new session in between. Two possible fixes: - Match the cleanup on the holder identity or `serverSessionId` instead of the key. - Keep a tombstone until `close()` completes, and only then drop it when reconnection is allowed. **A correction to [5]** My example there overstated the risk. A client that has already observed a server-side session id is still rejected with `INVALID_HANDLE.SESSION_CHANGED` even without a tombstone. What the tombstone really protects against is a newly created client object reusing the id, or a client that does not send `client_observed_server_side_session_id`. The concern about another process sharing the same id still stands. Could you share the concrete use case that requires a stable session id? That would help decide between (1) and (3). -- 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]
