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/13 11:55:56 UTC
[PR] [WIP][CORE] Replace Timer with single thread scheduled executor [spark]
beliefer opened a new pull request, #44718:
URL: https://github.com/apache/spark/pull/44718
### What changes were proposed in this pull request?
This PR propose to replace `Timer` with single thread scheduled executor.
### Why are the changes needed?
The javadoc recommends `ScheduledThreadPoolExecutor` instead of `Timer`.
This change based on the following two points.
**System time sensitivity**
Timer scheduling is based on the absolute time of the operating system and is sensitive to the operating system's time. Once the operating system's time changes, Timer scheduling is no longer precise.
The scheduled Thread Pool Executor scheduling is based on relative time and is not affected by changes in operating system time.
**Are anomalies captured**
Timer does not capture exceptions thrown by Timer Tasks, and in addition, Timer is single threaded. Once a scheduling task encounters an exception, the entire thread will terminate and other tasks that need to be scheduled will no longer be executed.
The scheduled Thread Pool Executor implements scheduling functions based on a thread pool. After a task throws an exception, other tasks can still execute normally.
### 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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1929098322
All test passed. Merged into master for Spark 4.0. Thanks @beliefer @srowen and @dongjoon-hyun ~
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1459024605
##########
core/src/main/scala/org/apache/spark/BarrierCoordinator.scala:
##########
@@ -51,7 +52,8 @@ private[spark] class BarrierCoordinator(
// TODO SPARK-25030 Create a Timer() in the mainClass submitted to SparkSubmit makes it unable to
// fetch result, we shall fix the issue.
- private lazy val timer = new Timer("BarrierCoordinator barrier epoch increment timer")
+ private lazy val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
The `purge` of `Timer` has the same name with the `purge` of `ThreadScheduledExecutor`.
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1925557802
Thank you, @beliefer .
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1459024605
##########
core/src/main/scala/org/apache/spark/BarrierCoordinator.scala:
##########
@@ -51,7 +52,8 @@ private[spark] class BarrierCoordinator(
// TODO SPARK-25030 Create a Timer() in the mainClass submitted to SparkSubmit makes it unable to
// fetch result, we shall fix the issue.
- private lazy val timer = new Timer("BarrierCoordinator barrier epoch increment timer")
+ private lazy val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
The `purge` of `Timer` has the same name with the purge of `ThreadScheduledExecutor`.
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1925468006
Thank you for updating. Could you make CI happy?
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1926413535
@dongjoon-hyun All the tests passed.
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1460887475
##########
core/src/main/scala/org/apache/spark/BarrierCoordinator.scala:
##########
@@ -51,7 +52,8 @@ private[spark] class BarrierCoordinator(
// TODO SPARK-25030 Create a Timer() in the mainClass submitted to SparkSubmit makes it unable to
// fetch result, we shall fix the issue.
- private lazy val timer = new Timer("BarrierCoordinator barrier epoch increment timer")
+ private lazy val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
Good idea.
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1478107179
##########
core/src/main/scala/org/apache/spark/BarrierTaskContext.scala:
##########
@@ -283,6 +284,7 @@ object BarrierTaskContext {
@Since("2.4.0")
def get(): BarrierTaskContext = TaskContext.get().asInstanceOf[BarrierTaskContext]
- private val timer = new Timer("Barrier task timer for barrier() calls.")
+ private val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
Hm I guess there is no place to shut this down. It's not critical and not something we have to change here. I wonder if it should be a daemon thread though? seems weird to spawn a thread that nothing in the code can terminate
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1459073865
##########
launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java:
##########
@@ -128,7 +125,8 @@ private LauncherServer() throws IOException {
this.threadIds = new AtomicLong();
this.factory = new NamedThreadFactory(THREAD_NAME_FMT);
this.secretToPendingApps = new ConcurrentHashMap<>();
- this.timeoutTimer = new Timer("LauncherServer-TimeoutTimer", true);
+ this.timeoutTimer = new ScheduledThreadPoolExecutor(
+ 1, new NamedThreadFactory("LauncherServer-TimeoutTimer"));
Review Comment:
nit: Indentation
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala:
##########
@@ -963,8 +964,8 @@ private[spark] class TaskSchedulerImpl(
barrierCoordinator.stop()
}
}
- starvationTimer.cancel()
- abortTimer.cancel()
+ starvationTimer.shutdown()
+ abortTimer.shutdown()
Review Comment:
It's fine, but would it be more appropriate to use `ThreadUtils.shutdown`? I am not sure.
##########
launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java:
##########
@@ -27,11 +27,8 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
-import java.util.Timer;
import java.util.TimerTask;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.*;
Review Comment:
I tend to avoid using `import *`
##########
launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java:
##########
@@ -27,11 +27,8 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
-import java.util.Timer;
import java.util.TimerTask;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.*;
Review Comment:
```suggestion
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
```
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1478197490
##########
core/src/main/scala/org/apache/spark/BarrierTaskContext.scala:
##########
@@ -283,6 +284,7 @@ object BarrierTaskContext {
@Since("2.4.0")
def get(): BarrierTaskContext = TaskContext.get().asInstanceOf[BarrierTaskContext]
- private val timer = new Timer("Barrier task timer for barrier() calls.")
+ private val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
Got it. Let's change it to daemon thread.
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1902558404
The GA failure is unrelated.
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1914619892
@dongjoon-hyun The GA failure is unrelated.
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1478139770
##########
core/src/main/scala/org/apache/spark/BarrierTaskContext.scala:
##########
@@ -283,6 +284,7 @@ object BarrierTaskContext {
@Since("2.4.0")
def get(): BarrierTaskContext = TaskContext.get().asInstanceOf[BarrierTaskContext]
- private val timer = new Timer("Barrier task timer for barrier() calls.")
+ private val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
The origin Timer is not a daemon thread, so I followed the same behavior.
There are some code used to cancel the timer task.
https://github.com/apache/spark/blob/db8c3058fb9d8e75f03dea6a821c50462315c513/core/src/main/scala/org/apache/spark/BarrierTaskContext.scala#L120
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1929117684
@LuciferYang @srowen @dongjoon-hyun 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] [SPARK-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #44718: [SPARK-46895][CORE] Replace Timer with single thread scheduled executor
URL: https://github.com/apache/spark/pull/44718
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1925550130
> Thank you for updating. Could you make CI happy?
I will try 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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1452314690
##########
core/src/main/scala/org/apache/spark/BarrierCoordinator.scala:
##########
@@ -168,7 +170,7 @@ private[spark] class BarrierCoordinator(
// we may timeout for the sync.
if (requesters.isEmpty) {
initTimerTask(this)
- timer.schedule(timerTask, timeoutInSecs * 1000)
+ timer.schedule(timerTask, timeoutInSecs * 1000, TimeUnit.MILLISECONDS)
Review Comment:
You can just remove "* 1000" and specify units as SECONDS
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1478158231
##########
core/src/main/scala/org/apache/spark/BarrierTaskContext.scala:
##########
@@ -283,6 +284,7 @@ object BarrierTaskContext {
@Since("2.4.0")
def get(): BarrierTaskContext = TaskContext.get().asInstanceOf[BarrierTaskContext]
- private val timer = new Timer("Barrier task timer for barrier() calls.")
+ private val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
That doesn't shut down the executor though, and the other code paths do. I don't see how the non-daemon thread ever terminates. It may not matter as the Spark context lives as long as the app. It'd be tidy to shut it down, or make it a daemon anyway, but not strictly related to the change you're making
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1460315231
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSchedulerImpl.scala:
##########
@@ -963,8 +964,8 @@ private[spark] class TaskSchedulerImpl(
barrierCoordinator.stop()
}
}
- starvationTimer.cancel()
- abortTimer.cancel()
+ starvationTimer.shutdown()
+ abortTimer.shutdown()
Review Comment:
Sounds good.
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1900054169
cc @dongjoon-hyun @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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1458907016
##########
core/src/main/scala/org/apache/spark/BarrierTaskContext.scala:
##########
@@ -71,7 +72,7 @@ class BarrierTaskContext private[spark] (
}
}
// Log the update of global sync every 60 seconds.
- timer.schedule(timerTask, 60000, 60000)
+ timer.scheduleAtFixedRate(timerTask, 60000, 60000, TimeUnit.MILLISECONDS)
Review Comment:
Same idea here, this is just 1 minute
##########
core/src/main/scala/org/apache/spark/BarrierCoordinator.scala:
##########
@@ -51,7 +52,8 @@ private[spark] class BarrierCoordinator(
// TODO SPARK-25030 Create a Timer() in the mainClass submitted to SparkSubmit makes it unable to
// fetch result, we shall fix the issue.
- private lazy val timer = new Timer("BarrierCoordinator barrier epoch increment timer")
+ private lazy val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
Doesn't cancelTimerTask() have to change too?
--
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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1891375189
@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-46698][CORE][FOLLOWUP] Replace Timer with single thread scheduled executor [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #44718:
URL: https://github.com/apache/spark/pull/44718#discussion_r1460840190
##########
core/src/main/scala/org/apache/spark/BarrierCoordinator.scala:
##########
@@ -51,7 +52,8 @@ private[spark] class BarrierCoordinator(
// TODO SPARK-25030 Create a Timer() in the mainClass submitted to SparkSubmit makes it unable to
// fetch result, we shall fix the issue.
- private lazy val timer = new Timer("BarrierCoordinator barrier epoch increment timer")
+ private lazy val timer = ThreadUtils.newSingleThreadScheduledExecutor(
Review Comment:
OK. I wonder if onStop() should also shutdown the timer? it isn't in the current code, but maybe it would be better to ensure it is stopped there
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1913995375
Thank you. Could you re-trigger the failed test pipelines?
--
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-46895][CORE] Replace Timer with single thread scheduled executor [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44718:
URL: https://github.com/apache/spark/pull/44718#issuecomment-1913813920
@dongjoon-hyun new jira ID created.
--
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