[GitHub] [spark] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming… URL: https://github.com/apache/spark/pull/25439#discussion_r317351660 ## File path: core/src/main/scala/org/apache/spark/ui/WebUI.scala ## @@ -93,6 +93,7 @@ private[spark] abstract class WebUI( attachHandler(renderJsonHandler) val handlers = pageToHandlers.getOrElseUpdate(page, ArrayBuffer[ServletContextHandler]()) handlers += renderHandler +handlers += renderJsonHandler Review comment: I think so, seems that it wasn't an issue in past cause other tabs can't be detached in runtime. 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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming… URL: https://github.com/apache/spark/pull/25439#discussion_r317154677 ## File path: core/src/main/scala/org/apache/spark/ui/WebUI.scala ## @@ -93,6 +93,7 @@ private[spark] abstract class WebUI( attachHandler(renderJsonHandler) val handlers = pageToHandlers.getOrElseUpdate(page, ArrayBuffer[ServletContextHandler]()) handlers += renderHandler +handlers += renderJsonHandler Review comment: Yes, handlers store reference to streaming and batch pages which store reference to ``StreamingJobProgressListener`` which stores reference to ``StreamingContext``. So all the handlers should be unregistered. 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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS]
- Fix StreamingContext leak through Streaming…
URL: https://github.com/apache/spark/pull/25439#discussion_r315885657
##
File path:
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala
##
@@ -26,37 +25,24 @@ import org.apache.spark.ui.{SparkUI, SparkUITab}
* Spark Web UI tab that shows statistics of a streaming job.
* This assumes the given SparkContext has enabled its SparkUI.
*/
-private[spark] class StreamingTab(val ssc: StreamingContext)
- extends SparkUITab(StreamingTab.getSparkUI(ssc), "streaming") with Logging {
-
- import StreamingTab._
+private[spark] class StreamingTab(val ssc: StreamingContext, sparkUI: SparkUI)
Review comment:
It's not necessary, but i believe it's a bit simpler and also consistent
with other ui tabs.
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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS]
- Fix StreamingContext leak through Streaming…
URL: https://github.com/apache/spark/pull/25439#discussion_r313720790
##
File path:
streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala
##
@@ -575,6 +577,8 @@ class StreamingContext private[streaming] (
try {
validate()
+registerProgressListener()
Review comment:
I think so. I believe``StreamingTab`` shouldn't be responsible for
registering/unregistering the listener as it could be and even already used in
other place (metrics). Moreover seems there is also a bug that if ui is
disabled, listener isn't registered and metrics aren't reported.
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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS]
- Fix StreamingContext leak through Streaming…
URL: https://github.com/apache/spark/pull/25439#discussion_r313719160
##
File path:
streaming/src/test/scala/org/apache/spark/streaming/InputStreamsSuite.scala
##
@@ -52,8 +52,6 @@ class InputStreamsSuite extends TestSuiteBase with
BeforeAndAfter {
// Set up the streaming context and input streams
withStreamingContext(new StreamingContext(conf, batchDuration)) { ssc =>
-ssc.addStreamingListener(ssc.progressListener)
-
Review comment:
Correct.
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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS]
- Fix StreamingContext leak through Streaming…
URL: https://github.com/apache/spark/pull/25439#discussion_r313719072
##
File path:
streaming/src/test/scala/org/apache/spark/streaming/StreamingContextSuite.scala
##
@@ -373,7 +374,7 @@ class StreamingContextSuite extends SparkFunSuite with
BeforeAndAfter with TimeL
Thread.sleep(100)
}
- test ("registering and de-registering of streamingSource") {
+ test("registering and de-registering of streamingSource") {
Review comment:
Got it, reverted.
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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS]
- Fix StreamingContext leak through Streaming…
URL: https://github.com/apache/spark/pull/25439#discussion_r313718918
##
File path: core/src/main/scala/org/apache/spark/ui/SparkUI.scala
##
@@ -138,6 +138,10 @@ private[spark] class SparkUI private (
streamingJobProgressListener = Option(sparkListener)
}
+ def clearStreamingJobProgressListener(): Unit = {
+streamingJobProgressListener = None
+ }
+
Review comment:
Removed
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] choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS] - Fix StreamingContext leak through Streaming…
choojoyq commented on a change in pull request #25439: [SPARK-28709][DSTREAMS]
- Fix StreamingContext leak through Streaming…
URL: https://github.com/apache/spark/pull/25439#discussion_r313718989
##
File path:
streaming/src/test/scala/org/apache/spark/streaming/StreamingContextSuite.scala
##
@@ -392,6 +393,29 @@ class StreamingContextSuite extends SparkFunSuite with
BeforeAndAfter with TimeL
assert(!sourcesAfterStop.contains(streamingSourceAfterStop))
}
+ test("registering and de-registering of progressListener") {
Review comment:
Sure, 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.
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
