[GitHub] [spark] HeartSaVioR edited a comment on issue #25705: [SPARK-29003][CORE] Fix deadlock in startup of Spark History Server
HeartSaVioR edited a comment on issue #25705: [SPARK-29003][CORE] Fix deadlock in startup of Spark History Server URL: https://github.com/apache/spark/pull/25705#issuecomment-530223614 > start() just waits for safe mode exit and for this suite of tests it is irrelevant. No. There're some initializations happening in `startPolling()`, and it's only called once we call `initialize()`. Even IS_TESTING is set, remaining initializations are executed. https://github.com/apache/spark/blob/7f36cd2aa5e066a807d498b8c51645b136f08a75/core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala#L243-L284 Safe mode exit is just one of functionalities in `initialize()`. So while we might be able to say it doesn't break the existing tests so it's kind of safe, it still brings side-effect. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HeartSaVioR edited a comment on issue #25705: [SPARK-29003][CORE] Fix deadlock in startup of Spark History Server
HeartSaVioR edited a comment on issue #25705: [SPARK-29003][CORE] Fix deadlock in startup of Spark History Server URL: https://github.com/apache/spark/pull/25705#issuecomment-530194631 Hmm... yes I agree it might be weird if we don't follow start/stop pattern. What I'm concerned about is the change would also make tests skip initializing. If the test calls `updateAndCheck()`, it misses initialization. It doesn't let existing tests be broken, but brings some unintentional side-effect. I think we even need to call `initialize` instead if it's weird for us to call `start` without calling `stop`, but given tests are passing, let's hear more voice on committers. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HeartSaVioR edited a comment on issue #25705: [SPARK-29003][CORE] Fix deadlock in startup of Spark History Server
HeartSaVioR edited a comment on issue #25705: [SPARK-29003][CORE] Fix deadlock in startup of Spark History Server URL: https://github.com/apache/spark/pull/25705#issuecomment-530161932 You can find all implementations for ApplicationHistoryProvider in IDE, for me, IntelliJ. Other than FsHistoryProvider, they're in test suites. Seems like you missed to add start() for all places initializing FsHistoryProvider, which means calling startPolling() is missing: for testing, background threads will not be launched so that's OK, but initializing disk manager and validating log directory are missing. Test suites don't concern about it so test passes (I'd say it's luck), but given it's unintentional to not add start(), we would be better to fix 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
