zhengruifeng commented on PR #58085: URL: https://github.com/apache/spark/pull/58085#issuecomment-5390219025
I reviewed the deadline change manually despite the current merge conflicts. The dedicated \ deadline looks reasonable to me. \ is sent over \, but semantically it is best-effort cleanup from \, not normal query execution. I don't think this should reuse the regular non-reattachable \ behavior, because adding a deadline there could abandon real query work without a recovery path. I also would not reuse \: that releases the whole Connect session and has a different scope/default than releasing one cached remote relation from a finalizer. One related follow-up to consider: \ in \ is also best-effort cleanup and currently calls the stub without a timeout. It is less urgent because it runs asynchronously in the release threadpool rather than directly on the foreachBatch critical path, but it may deserve a similar bounded deadline in this deadline model. I also ran the focused new test and the existing \ default/disabled/non-positive tests on the PR head; they passed. -- 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]
