You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2024/01/15 06:48:53 UTC
[PR] [WIP][CORE] Simplify the ContextCleaner by the exit operation only depend on interrupt thread. [spark]
beliefer opened a new pull request, #44732:
URL: https://github.com/apache/spark/pull/44732
### What changes were proposed in this pull request?
This PR propose to simplify the `ContextCleaner`.
### Why are the changes needed?
Currently, close or destroy `ContextCleaner` depend on interrupt thread and the volatile variable `stopped`.
In fact, we can change the `stopped` to a local variable on stack and let the close operation of `ContextCleaner` only depend on interrupt thread.
For further optimization, this PR using `running` instead of `stopped`.
### Does this PR introduce _any_ user-facing change?
'No'.
### How was this patch tested?
GA tests.
### Was this patch authored or co-authored using generative AI tooling?
'No'.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1905634936
Since you looked into this - do you have any other thoughts on this @LuciferYang ? Thanks
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1894969724
@dongjoon-hyun @srowen @LuciferYang
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1907523240
Merged to master.
Thanks for working on this @beliefer !
Thanks for the review @LuciferYang :-)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1894984377
This change of this pr is fine to me.
However, I would like to raise a question: Would it be simpler and clearer to implement the logic for cleaning up the `DownloadFile` resource using the `java.lang.ref.Cleaner` mechanism instead of the current approach?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1893757819
> Unlike `ReloadingX509TrustManager `, here the `InterruptedException` is caught and ignored in some of the downstream methods: so we cant rely on that pattern here.
I reverted the `ContextCleaner`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #44732: [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread.
URL: https://github.com/apache/spark/pull/44732
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1907403108
no more, the change is fine to me
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1895487153
`Java.lang.ref.Cleaner` cannot respond to interrupts.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1905176247
cc @mridulm
Could you take a review again?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1907918969
@mridulm @LuciferYang Thank you!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][CORE] Simplify the ContextCleaner|BlockManager by the exit operation only depend on interrupt thread. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44732:
URL: https://github.com/apache/spark/pull/44732#issuecomment-1893296284
> Unlike `ReloadingX509TrustManager `, here the `InterruptedException` is caught and ignored in some of the downstream methods: so we cant rely on that pattern here.
`ContextCleaner` catches `InterruptedException` but ignore it. How can downstream capture it?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org