Uh oh!
There was an error while loading. Please reload this page.
[SPARK-23608][CORE][WebUI] Add synchronization in SHS between attachSparkUI and detachSparkUI functions to avoid concurrent modification issue to Jetty Handlers - #20744
Conversation
…parkUI and detachSparkUI functions to avoid concurrent modification issue to Jetty Handlers.
| /** Detach a reconstructed UI from this server. Only valid after bind(). */ | ||
| override def detachSparkUI(appId: String, attemptId: Option[String], ui: SparkUI): Unit = { | ||
| override def detachSparkUI(appId: String, attemptId: Option[String], ui: SparkUI): Unit = this.synchronized { |
There was a problem hiding this comment.
I'll trigger tests but this will cause a style violation.
There was a problem hiding this comment.
Thanks. Will fix the style violation.
vanzin
commented
Mar 6, 2018
ok to test |
SparkQA
commented
Mar 6, 2018
Test build #88018 has finished for PR 20744 at commit
|
vanzin
commented
Mar 8, 2018
@zhouyejoe still waiting for the fix... |
zhouyejoe
commented
Mar 8, 2018
Updated. Please help trigger the jenkins again. Thanks. |
SparkQA
commented
Mar 9, 2018
Test build #88105 has finished for PR 20744 at commit
|
| attemptId: Option[String], | ||
| ui: SparkUI, | ||
| completed: Boolean) { | ||
| completed: Boolean): Unit = this.synchronized { |
There was a problem hiding this comment.
Hmm... would it be better to synchronize SparkUI.attachHandler and SparkUI.detachHandler? Or maybe restrict the scope of the synchronization here?
This seems too coarse; in the detach case, it synchronizes around the onUIDetached callback which can do expensive locking and I/O.
There was a problem hiding this comment.
In order to fine grain the synchronization, can we add synchronize on handlers only?
SparkQA
commented
Mar 13, 2018
Test build #88187 has finished for PR 20744 at commit
|
| handlers.synchronized { | ||
| ui.getHandlers.foreach(attachHandler) | ||
| } | ||
| addFilters(ui.getHandlers, conf) |
There was a problem hiding this comment.
shall we also keep handlers synchronized for this line?
There was a problem hiding this comment.
Yes. Thanks for catching this. Will update.
SparkQA
commented
Mar 14, 2018
Test build #88237 has finished for PR 20744 at commit
|
zhouyejoe
commented
Mar 15, 2018
It is weird that the PySpark unit tests failed, I don't think it is related. |
vanzin
commented
Mar 16, 2018
This doesn't have anything to do with pyspark, so we can ignore those. Merging to master / 2.3. |
…parkUI and detachSparkUI functions to avoid concurrent modification issue to Jetty Handlers Jetty handlers are dynamically attached/detached while SHS is running. But the attach and detach operations might be taking place at the same time due to the async in load/clear in Guava Cache. ## What changes were proposed in this pull request? Add synchronization between attachSparkUI and detachSparkUI in SHS. ## How was this patch tested? With this patch, the jetty handlers missing issue never happens again in our production cluster SHS. Author: Ye Zhou <yezhou@linkedin.com> Closes#20744 from zhouyejoe/SPARK-23608. (cherry picked from commit 3675af7) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
…parkUI and detachSparkUI functions to avoid concurrent modification issue to Jetty Handlers Jetty handlers are dynamically attached/detached while SHS is running. But the attach and detach operations might be taking place at the same time due to the async in load/clear in Guava Cache. ## What changes were proposed in this pull request? Add synchronization between attachSparkUI and detachSparkUI in SHS. ## How was this patch tested? With this patch, the jetty handlers missing issue never happens again in our production cluster SHS. Author: Ye Zhou <yezhou@linkedin.com> Closesapache#20744 from zhouyejoe/SPARK-23608.
…parkUI and detachSparkUI functions to avoid concurrent modification issue to Jetty Handlers Jetty handlers are dynamically attached/detached while SHS is running. But the attach and detach operations might be taking place at the same time due to the async in load/clear in Guava Cache. ## What changes were proposed in this pull request? Add synchronization between attachSparkUI and detachSparkUI in SHS. ## How was this patch tested? With this patch, the jetty handlers missing issue never happens again in our production cluster SHS. Author: Ye Zhou <yezhou@linkedin.com> Closesapache#20744 from zhouyejoe/SPARK-23608. (cherry picked from commit 3675af7) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
Jetty handlers are dynamically attached/detached while SHS is running. But the attach and detach operations might be taking place at the same time due to the async in load/clear in Guava Cache.
What changes were proposed in this pull request?
Add synchronization between attachSparkUI and detachSparkUI in SHS.
How was this patch tested?
With this patch, the jetty handlers missing issue never happens again in our production cluster SHS.