venkata91 commented on PR #59277: URL: https://github.com/apache/spark/pull/59277#issuecomment-6434810759
@dongjoon-hyun, thanks again for the review on Oct 7. I reworked the PR around your comments, so it no longer changes `ReleaseSession(allow_reconnect=true)` or the Python `release_session` path. Here is where each point stands: | # | Your point | Status | |---|---|---| | 1 | Tombstone dropped while the old session is still closing | Fixed. The id stays fenced (`SESSION_CLOSED`) until `SessionHolder.close()` finishes, using a separate `closingSessions` map (7853a51d062, f2ebb5a9e54). Covered by "session id stays tombstoned until the owning close completes", for both values of the conf. | | 2 | A concurrent shutdown re-puts the invalidated tombstone | Fixed. Only the caller that atomically removes the holder (`tryRemoveSessionHolder`) closes it and writes its tombstone, so nothing re-puts a tombstone after another caller (7853a51d062, 710d9bb5e27). | | 3 | The old client can close the recreated session | Documented in the conf doc and `docs/configuration.md`: don't stop the old `SparkSession` once another client may have reattached. A real fix needs `ReleaseSessionRequest` to carry the observed server-side session id. That is a proto change, so I'd like to do it in a follow-up JIRA, as discussed with @HyukjinKwon. | | 4 | Stale `_server_session_id` and an orphan session | Fixed. A request that carries an observed server-side session id fails with `SESSION_CHANGED` before any session is created, for both lookups and get-or-create (b4f98b9d378, 42cc9b6fbd8). The clients now also send that id on artifact RPCs (8496d1ff41b). | | 5 | Any caller can lift the fence; no opt-out | The fence is no longer lifted by `ReleaseSession`. Reuse needs the session to opt in with `spark.connect.session.allowReconnectAfterClose` (default `false`). Without it, closed ids stay tombstoned as before. | | 6–8 | Test and cleanup nits | The tests and code these referred to were replaced in the rework. The new tests check the exact error conditions and that no session is created, and they cover the in-close window, a throwing `close()`, and an artifact RPC sent through the real handler. | The red check is `KafkaRealTimeModeWindowSuite` in `sql-kafka-0-10`, which is unrelated to this PR. I've rerun it. Could you take another look and update your review when you have a chance? cc @HyukjinKwon -- 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]
