You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhouyifan279 (via GitHub)" <gi...@apache.org> on 2023/05/09 15:53:32 UTC
[GitHub] [spark] zhouyifan279 opened a new pull request, #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
zhouyifan279 opened a new pull request, #41105:
URL: https://github.com/apache/spark/pull/41105
### What changes were proposed in this pull request?
Ensure old SparkUI in HistoryServer has been detached before loading new one.
### Why are the changes needed?
We encountered same error described in SPARK-43403 in two different SparkHistoryServers.
It happened when `FsHistoryProvider#onUIDetached` run (in thread-a) right after a new SparkUI with the same appId and attemptId was loaded (in thread-b).
To avoid this, we let loading of the new SparkUI wait until old SparkUI is detached.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Add test `Load new SparkUI before old one detached` in `ApplicationCacheSuite`
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1190592755
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
+ throw new TimeoutException("Timed out waiting for old SparkUI to be detached")
Review Comment:
More places in Spark directly use `java.util.concurrent.TimeoutException` instead of `scala.concurrent.TimeoutException`(`s.c.TimeoutException` is just an alias for j.u.c.TimeoutException).
--
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-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1905798079
@LuciferYang @yaooqinn Created a new PR #44851
--
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
[GitHub] [spark] mridulm commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1191801323
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
removal essentially can be held up by a few synchronized blocks - do we have any stats on how long this takes ?
How did we end up with < 1 s estimate ?
--
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
[GitHub] [spark] mridulm commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1544930839
+CC @thejdeep
--
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
[GitHub] [spark] mridulm commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1194654288
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
To clarify, `load` is now going to wait for the `onRemoval` to complete - and going down `detachSparkUI` call tree, there are `synchronized` blocks in some of the subtrees.
I am unsure how contended these might be, how expensive some of those operations are (closing the store, for example), what the potential time taken are ... and this was the context of my query around whether 10s is sufficient/too little/too high ?
--
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
[GitHub] [spark] zhouyifan279 commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1541576100
@srowen @LuciferYang Added some code segments to help to understand this issue.
--
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
[GitHub] [spark] mridulm commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1194654288
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
To clarify, `load` is now going to wait for the `onRemoval` to complete - and going down `detachSparkUI` call tree, there are `synchronized` blocks in some of the subtrees.
I am unsure how contended these might be, what the potential time taken are ... and this was the context of my query around whether 10s is sufficient/too little/too high ?
--
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
[GitHub] [spark] steveloughran commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1542317728
don't remember the cache stuff, sorry. I know we have a problem in hadoop where cache lookup can trigger >1 fs creation and the same time, and if that is slow then at best: needless work, at worst: conflict and sometimes failures. so we use a semaphore to limit the #of threads which can create a new FS at the same time (HADOOP-17313). spark/tez workers and cloud stores doing network IO in initialize() are the troublespot here, FWIW.
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1299018994
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
After deploy this patch in our product environment, TimeoutException was thrown occasionally when opening an active Spark UI in HistoryServer. It happened in Application granularity and lasted for minutes.
Haven't figured out what causes this.
--
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][SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1847023452
> After deploy this patch in our product environment, TimeoutException was thrown occasionally when opening a running Spark UI in HistoryServer. It happened in Application granularity and lasted for minutes.
>
> Haven't figured out what causes this.
Issue described in [SPARK-46330](https://issues.apache.org/jira/browse/SPARK-46330) can cause `TimeoutException`.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189473010
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
@zhouyifan279 By the way, does RocksDB backend also have the issue you pointed out?
--
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][SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #41105: [WIP][SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
URL: https://github.com/apache/spark/pull/41105
--
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
[GitHub] [spark] zhouyifan279 commented on pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1540504315
cc @srowen
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189470862
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
I have some questions about this. Shouldn't the `CacheLoader#load` method only be executed once for accessing the same key? Why is there a scenario where the same key enters the load method more than once?
Is this a bug of Guava 14.0.1 or did I have an incorrect understanding of the mechanism of `CacheLoader`? Please correct me.
In addition, from the Jira description, does this issue only exist in Spark 3.1.2?
also cc @steveloughran FYI due to he is the author of `ApplicationCache`.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189503375
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
@zhouyifan279 After reading the pr description again with the new UT, I understand your intention. Let me think more about 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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1191865529
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
> removal essentially can be held up by a few synchronized blocks
Can you provide an example? From what I know, removal are mostly triggered by `ApplicationCache#withSparkUI`, which only causes http requests to the same <appId, attemptId> being blocked.
> do we have any stats on how long this takes ? How did we end up with < 1 s estimate ?
There is no heavy operation in the code path of `ApplicationCache#removalListener#onRemoval`.
The most heavy operation is deleting app data directory on disk.
I created 10,000 files in a directory and use `rm -rf` to delete it. Deletion takes 0.339s.
So I think 1 seconds should be enough.
--
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
[GitHub] [spark] zhouyifan279 commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1543275831
> don't remember the cache stuff, sorry. I know we have a problem in hadoop where cache lookup can trigger >1 fs creation and the same time, and if that is slow then at best: needless work, at worst: conflict and sometimes failures. so we use a semaphore to limit the #of threads which can create a new FS at the same time (HADOOP-17313). spark/tez workers and cloud stores doing network IO in initialize() are the troublespot here, FWIW.
@steveloughran Thanks for your inputs.
In SparkHistoryServer, SparkUIs are tracked by <appId, attemptId> in many places:
1. `ApplicationCache#appCache`
2. `FsHistoryProvider#activeUIs`
3. `HistoryServerDiskManager#active`
4. Disk-based KVStore backend local path: appStoreDir/<appId>_<attemptId>/
All the above places assumes there is only one SparkUI loaded for one <appId, attemptId> pair.
Guava LoadingCache can ensure this when SparkUIs are guarded by its locks.
But SparkUI's detaching is not guarded by LoadingCache's lock.
This PR is aimed to ensure this from SparkUI's loading to detaching.
--
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
[GitHub] [spark] zhouyifan279 commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1541571622
Here are some key code segments:
1. Detaching of old SparkUI is called when it is removed from `appCache`
**ApplicationCache**
```scala
private val removalListener = new RemovalListener[CacheKey, CacheEntry] {
/**
* Removal event notifies the provider to detach the UI.
* @param rm removal notification
*/
override def onRemoval(rm: RemovalNotification[CacheKey, CacheEntry]): Unit = {
metrics.evictionCount.inc()
val key = rm.getKey
logDebug(s"Evicting entry ${key}")
operations.detachSparkUI(key.appId, key.attemptId, rm.getValue().loadedUI.ui)
}
}
```
2. Execution of `removalListener#onRemoval` is not guarded by `appCache`'s lock
**com.google.common.cache.LocalCache.Segment**
```java
V lockedGetOrLoad(K key, int hash, CacheLoader<? super K, V> loader)
throws ExecutionException {
ReferenceEntry<K, V> e;
ValueReference<K, V> valueReference = null;
LoadingValueReference<K, V> loadingValueReference = null;
boolean createNewEntry = true;
lock();
try {
...
} finally {
unlock();
// `removalListener#onRemoval` executes inside postWriteCleanup()
postWriteCleanup();
}
...
}
```
3. New SparkUI is detached during detaching of old SparkUI
**FsHistoryProvider**
```scala
override def onUIDetached(appId: String, attemptId: Option[String], ui: SparkUI): Unit = {
val uiOption = synchronized {
// After new SparkUI is loaded, `activeUIs` contains new SparkUI
activeUIs.remove((appId, attemptId))
}
uiOption.foreach { loadedUI =>
loadedUI.lock.writeLock().lock()
try {
loadedUI.ui.store.close()
} finally {
loadedUI.lock.writeLock().unlock()
}
diskManager.foreach { dm =>
// If the UI is not valid, delete its files from disk, if any. This relies on the fact that
// ApplicationCache will never call this method concurrently with getAppUI() for the same
// appId / attemptId.
dm.release(appId, attemptId, delete = !loadedUI.valid)
}
}
}
```
--
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-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1905790952
@zhouyifan279 Seems you need submit a new one.
--
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][SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1847080341
@LuciferYang @mridulm because of issue [SPARK-46330](https://issues.apache.org/jira/browse/SPARK-46330), I removed the timeout parameter in waiting old Spark UI detaching.
Can you reopen this PR and review it 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] [WIP][SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1835054687
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1905735349
I'm not able to do that
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189272437
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,24 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ removalLatch.await()
Review Comment:
OK
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189512056
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
> @zhouyifan279 By the way, does RocksDB backend also have the issue you pointed out?
Active SparkUI's KVStore is closed when this issue occurs. So RocksDB backend also have the issue.
--
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
[GitHub] [spark] zhouyifan279 commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1544115380
@LuciferYang Exception "java.lang.IllegalStateException: DB is closed." can be reproduced in test now.
<img width="1138" alt="image" src="https://github.com/apache/spark/assets/88070094/cea0d752-4323-40ef-b4d1-5ec457fc267e">
--
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-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one [spark]
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1905704227
@yaooqinn Because old SparkUI detaching time can be unpredictable (explained in [SPARK-46330](https://issues.apache.org/jira/browse/SPARK-46330)), I removed the timeout limit for waiting old Spark UI detaching.
Can you reopen this PR and review it 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
[GitHub] [spark] zhouyifan279 commented on pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1541270960
cc @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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189507968
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
> I have some questions about this. Shouldn't the `CacheLoader#load` method only be executed once for accessing the same key? Why is there a scenario where the same key enters the load method more than once?
>
> Is this a bug of Guava 14.0.1 or did I have an incorrect understanding of the mechanism of `CacheLoader`? Please correct me.
There is no bug on "`CacheLoader#load` method only be executed once for accessing the same key".
The CountDownLatch is used to wait the completion of `RemovalListener#onRemoval` of the old SparkUI with the same key.
`RemovalListener#onRemoval` and `CacheLoader#load` can execute concurrently because `RemovalListener#onRemoval` is not guarded by LoadingCache's lock for keys.
> In addition, from the Jira description, does this issue only exist in Spark 3.1.2?
We found this issue in Spark 3.1.2 . But it should exists in a wide range of Spark versions.
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1190611255
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
+ throw new TimeoutException("Timed out waiting for old SparkUI to be detached")
Review Comment:
Updated
--
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
[GitHub] [spark] HyukjinKwon commented on pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41105:
URL: https://github.com/apache/spark/pull/41105#issuecomment-1547067573
cc @gengliangwang and @sarutak too FYI
--
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
[GitHub] [spark] srowen commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189017623
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,24 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ removalLatch.await()
Review Comment:
How about a timeout here just to be safe?
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189507968
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
> I have some questions about this. Shouldn't the `CacheLoader#load` method only be executed once for accessing the same key? Why is there a scenario where the same key enters the load method more than once?
>
> Is this a bug of Guava 14.0.1 or did I have an incorrect understanding of the mechanism of `CacheLoader`? Please correct me.
There is no bug on "`CacheLoader#load` method only be executed once for accessing the same key".
The CountDownLatch is used to wait the completion of `RemovalListener#onRemoval` of the old SparkUI with the same key.
`RemovalListener#onRemoval` and `CacheLoader#load` can execute concurrently because `RemovalListener#onRemoval` is not guarded by LoadingCache's lock for keys.
> In addition, from the Jira description, does this issue only exist in Spark 3.1.2?
We found this issue in Spark 3.1.2 . But it should exists in a wide range of Spark versions.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #41105: [SPARK-43403] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1189510787
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +49,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
Review Comment:
Yeah, already understand. Let me think more about this fix
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1191865529
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
> removal essentially can be held up by a few synchronized blocks
Can you provide an example? From what I know, removal are almost triggered by `ApplicationCache#withSparkUI`, which only causes http requests to the same <appId, attemptId> being blocked.
> do we have any stats on how long this takes ? How did we end up with < 1 s estimate ?
There is no heavy operation in the code path of `ApplicationCache#removalListener#onRemoval`.
The most heavy operation is deleting app data directory on disk.
I created 10,000 files in a directory and use `rm -rf` to delete it. Deletion takes 0.339s.
So I think 1 seconds should be enough.
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1191153894
##########
core/src/test/scala/org/apache/spark/deploy/history/ApplicationCacheSuite.scala:
##########
@@ -381,4 +385,38 @@ class ApplicationCacheSuite extends SparkFunSuite with MockitoSugar with Matcher
verify(resp).sendRedirect("http://localhost:18080/history/local-123/jobs/job/?id=2")
}
+ test("Load new SparkUI during old one is detaching") {
Review Comment:
OK. Let me try.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #41105: [SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1190842120
##########
core/src/test/scala/org/apache/spark/deploy/history/ApplicationCacheSuite.scala:
##########
@@ -381,4 +385,38 @@ class ApplicationCacheSuite extends SparkFunSuite with MockitoSugar with Matcher
verify(resp).sendRedirect("http://localhost:18080/history/local-123/jobs/job/?id=2")
}
+ test("Load new SparkUI during old one is detaching") {
Review Comment:
The new case should named as `SPARK-43403: ... xxx`.
And I think it would be best if the new case could be reproduced `java.lang.IllegalStateException: DB is closed` without change of `ApplicationCache` and pass with this pr.
If it is not easy to reproduce `DB is closed` (without change of ApplicationCache) in this one, I think you can add other `test`
--
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
[GitHub] [spark] zhouyifan279 commented on a diff in pull request #41105: [WIP][SPARK-43403][UI] Ensure old SparkUI in HistoryServer has been detached before loading new one
Posted by "zhouyifan279 (via GitHub)" <gi...@apache.org>.
zhouyifan279 commented on code in PR #41105:
URL: https://github.com/apache/spark/pull/41105#discussion_r1299018994
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
After deploy this patch in our product environment, TimeoutException was thrown occasionally when opening an running Spark UI in HistoryServer. It happened in Application granularity and lasted for minutes.
Haven't figured out what causes this.
##########
core/src/main/scala/org/apache/spark/deploy/history/ApplicationCache.scala:
##########
@@ -48,11 +48,28 @@ private[history] class ApplicationCache(
val retainedApplications: Int,
val clock: Clock) extends Logging {
+ /**
+ * Keep track of SparkUIs in [[ApplicationCache#appCache]] and SparkUIs removed from
+ * [[ApplicationCache#appCache]] but not detached yet.
+ */
+ private val loadedApps = new ConcurrentHashMap[CacheKey, CountDownLatch]()
+
private val appLoader = new CacheLoader[CacheKey, CacheEntry] {
/** the cache key doesn't match a cached entry, or the entry is out-of-date, so load it. */
override def load(key: CacheKey): CacheEntry = {
- loadApplicationEntry(key.appId, key.attemptId)
+ // Ensure old SparkUI has been detached before loading new one.
+ val removalLatch = loadedApps.get(key)
+ if (removalLatch != null) {
+ // Old SparkUI is in the middle of detaching.
+ // Waiting 10 seconds should be enough since detaching usually takes less than 1 second.
+ if (!removalLatch.await(10, TimeUnit.SECONDS)) {
Review Comment:
After deploy this patch in our product environment, TimeoutException was thrown occasionally when opening a running Spark UI in HistoryServer. It happened in Application granularity and lasted for minutes.
Haven't figured out what causes this.
--
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