fix task scheduler bugs: dead lock and dup server-info #1079

javeme · 2020-07-03T15:03:23Z

fix dead lock when scheduler-worker hold scheduleTasks lock then
query tasks, tx() method synchronized(this) wait indefinitely.
delete server-info when graph close (when store is initialized)
throw exception if task if failed in syncWait (just for debug tinkerpop tests)
submit task if there is only single node
standardize the status of tasks (add scheduling/scheduled)
cancel-task don't remove task from memory if it's single-node server
notifyNewTask only notify when the queue is empty(size=1 means one timer task)
read/write server-infos in bache duting one scheduling

fix #1075
Change-Id: Icb8d5073f024f0598d439d738d6c0e2caa6fd0cd

1.fix dead lock when scheduler-worker hold scheduleTasks lock then query tasks, tx() method synchronized(this) wait indefinitely. 2.delete server-info when graph close 3.throw exception if task if failed in syncWait fix #1075 Change-Id: Icb8d5073f024f0598d439d738d6c0e2caa6fd0cd

Change-Id: I69efd7d48610619379975e7d284e5dab88e30776

* standardize the status of tasks * remove server info only after storage is initialized * cancel(task) don't remove task from memory if it's single-node server * notifyNewTask only notify when the queue is empty Change-Id: I874724519e516f600ee8eb1b4760a607421a844a

codecov · 2020-07-07T15:01:37Z

Codecov Report

Merging #1079 into master will increase coverage by 0.22%.
The diff coverage is 65.13%.

@@             Coverage Diff              @@
##             master    #1079      +/-   ##
============================================
+ Coverage     68.94%   69.16%   +0.22%     
- Complexity     5543     5670     +127     
============================================
  Files           331      331              
  Lines         26880    27260     +380     
  Branches       3842     3986     +144     
============================================
+ Hits          18533    18855     +322     
- Misses         6532     6558      +26     
- Partials       1815     1847      +32

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/com/baidu/hugegraph/job/JobBuilder.java	`100.00% <ø> (ø)`	`12.00 <0.00> (ø)`
...c/main/java/com/baidu/hugegraph/task/HugeTask.java	`69.75% <38.46%> (-2.58%)`	`81.00 <1.00> (+2.00)`	⬇️
...va/com/baidu/hugegraph/task/ServerInfoManager.java	`71.09% <62.19%> (-7.29%)`	`46.00 <23.00> (+14.00)`	⬇️
...om/baidu/hugegraph/task/StandardTaskScheduler.java	`79.34% <62.22%> (-1.81%)`	`89.00 <18.00> (+2.00)`	⬇️
...ain/java/com/baidu/hugegraph/task/TaskManager.java	`73.68% <80.00%> (-0.95%)`	`26.00 <5.00> (ø)`
...in/java/com/baidu/hugegraph/StandardHugeGraph.java	`80.10% <100.00%> (-0.26%)`	`108.00 <0.00> (ø)`
...ugegraph/backend/store/memory/InMemoryDBTable.java	`67.60% <100.00%> (+0.46%)`	`42.00 <0.00> (ø)`
.../java/com/baidu/hugegraph/task/HugeServerInfo.java	`68.38% <100.00%> (-0.32%)`	`18.00 <3.00> (ø)`
...main/java/com/baidu/hugegraph/task/TaskStatus.java	`95.83% <100.00%> (+0.37%)`	`6.00 <0.00> (ø)`
...hugegraph/job/schema/IndexLabelRemoveCallable.java	`88.88% <0.00%> (-11.12%)`	`4.00% <0.00%> (-1.00%)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82d1e51...21cb8c1. Read the comment docs.

houzhizhen · 2020-07-08T00:46:25Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/TaskManager.java

@@ -242,17 +237,19 @@ public int pendingTasks() {
    }

    protected void notifyNewTask(HugeTask<?> task) {


Parameter task is not used.

does not matter, just preserved

houzhizhen · 2020-07-08T01:08:42Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/TaskManager.java

-        this.schedulerExecutor.submit(this::scheduleOrExecuteJob);
+        Queue<Runnable> queue = ((ThreadPoolExecutor) this.schedulerExecutor)
+                                                          .getQueue();
+        if (queue.size() <= 1) {


It may schedule more tasks at the same time, and these tasks will not be processed.

it's ok to not notify again if there are more than 1 task in queue(like two, one is timer task, one is immediate task), we don't want too many immediate tasks to be inserted into queue, one notify will cause all the tasks processed.

zhoney · 2020-07-08T03:00:23Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/StandardTaskScheduler.java

-    protected synchronized <V> void scheduleTasks() {
-        // Master schedule all queued tasks to suitable servers
+    protected synchronized void scheduleTasks() {
+        // Master server schedule all queued tasks to suitable worker servers


queued => scheduling

zhoney · 2020-07-08T03:10:30Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/StandardTaskScheduler.java

                server.load(server.load() + task.load());
-                this.serverManager().save(server);
-                LOG.info("Schedule task {} to server {}",
+


must save server info here. Otherwise all tasks will be scheduled to same worker

Implement increaseLoad(int load) in class HugeServerInfo

use batch read write

zhoney · 2020-07-08T03:30:36Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/StandardTaskScheduler.java

@@ -377,15 +402,14 @@ private void unlistenChanges() {
                 * initialized when canceled.
                 */
                this.initTaskCallable(task);
-                @SuppressWarnings("unchecked")
-                HugeTask<V> memTask = (HugeTask<V>) this.tasks.get(task.id());
+                HugeTask<?> memTask = this.tasks.get(task.id());
                if (memTask != null) {
                    task = memTask;
                }
                Id taskServer = task.server();
                if (taskServer != null && taskServer.equals(server)) {


move to line 397 and change to:

Id taskServer = task.server(); assert taskServer != null; if (!taskServer.equals(server)) { continue; }

zhoney · 2020-07-08T03:43:31Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/ServerInfoManager.java

@@ -212,8 +273,8 @@ protected Id save(HugeServerInfo server) {
        });


remove line 218 due to system tx is auto commit

Linary · 2020-07-08T04:48:14Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/StandardTaskScheduler.java

+             * this lock through scheduleTasks(), then query tasks and wait
+             * for db-worker thread after call(), the tx may not be initialized
+             * but can't catch this lock, then cause dead lock.
+             * We just use this.eventListener ad a monitor here


as a monitor

Change-Id: I23c845ee93c05ebec1fbea77fdc36a5cdb2614c2

javeme · 2020-07-08T08:53:55Z

zhoney · 2020-07-09T07:38:10Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/StandardTaskScheduler.java

        E.checkArgumentNotNull(task, "Task can't be null");
+        this.checkOnMasterNode("schedule");


move to line 234

zhoney · 2020-07-09T08:12:56Z

hugegraph-core/src/main/java/com/baidu/hugegraph/task/StandardTaskScheduler.java

+                 * is running but removed from memory.
+                 */
+                this.remove(task);
+            }


else means task is on master, cancel it directly seems more efficient

We assume cancel is not a frequent operation, so it is not optimized

Change-Id: I74a0164fd755bae885575a1770995502def6e88b

1. fix dead lock when scheduler-worker hold scheduleTasks lock then query tasks, tx() method synchronized(this) wait indefinitely. 2. delete server-info when graph close (when store is initialized) 3. throw exception if task if failed in syncWait (just for debug tinkerpop tests) 4. submit task if there is only single node 5. standardize the status of tasks (add scheduling/scheduled) 6. cancel-task don't remove task from memory if it's single-node server 7. notifyNewTask only notify when the queue is empty(size=1 means one timer task) 8. read/write server-infos in bache duting one scheduling fix #1075 Change-Id: Icb8d5073f024f0598d439d738d6c0e2caa6fd0cd

javeme added 3 commits July 7, 2020 22:41

submit task if there is only single node

600df25

Change-Id: I69efd7d48610619379975e7d284e5dab88e30776

fix some bugs

795ef89

* standardize the status of tasks * remove server info only after storage is initialized * cancel(task) don't remove task from memory if it's single-node server * notifyNewTask only notify when the queue is empty Change-Id: I874724519e516f600ee8eb1b4760a607421a844a

javeme force-pushed the schedule-task-bug-fix branch from 84875cc to 795ef89 Compare July 7, 2020 14:41

houzhizhen suggested changes Jul 8, 2020

View reviewed changes

houzhizhen reviewed Jul 8, 2020

View reviewed changes

zhoney requested changes Jul 8, 2020

View reviewed changes

Linary requested changes Jul 8, 2020

View reviewed changes

javeme force-pushed the schedule-task-bug-fix branch from 69ddc1f to cad38cf Compare July 8, 2020 08:47

read/write server-infos in bache duting one scheduling

176c988

Change-Id: I23c845ee93c05ebec1fbea77fdc36a5cdb2614c2

javeme force-pushed the schedule-task-bug-fix branch from cad38cf to 176c988 Compare July 8, 2020 08:48

javeme mentioned this pull request Jul 8, 2020

Async job schedule #1030

Merged

houzhizhen previously approved these changes Jul 8, 2020

View reviewed changes

zhoney requested changes Jul 9, 2020

View reviewed changes

fix not check master with EphemeralJob and fix onlySingleNode update

21cb8c1

Change-Id: I74a0164fd755bae885575a1770995502def6e88b

javeme dismissed houzhizhen’s stale review via 21cb8c1 July 9, 2020 12:36

zhoney approved these changes Jul 10, 2020

View reviewed changes

houzhizhen approved these changes Jul 10, 2020

View reviewed changes

Linary approved these changes Jul 10, 2020

View reviewed changes

Linary merged commit 3c358f8 into master Jul 10, 2020

Linary deleted the schedule-task-bug-fix branch July 10, 2020 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix task scheduler bugs: dead lock and dup server-info #1079

fix task scheduler bugs: dead lock and dup server-info #1079

javeme commented Jul 3, 2020 •

edited

codecov bot commented Jul 7, 2020 •

edited

houzhizhen Jul 8, 2020

javeme Jul 8, 2020

houzhizhen Jul 8, 2020

javeme Jul 8, 2020

zhoney Jul 8, 2020

zhoney Jul 8, 2020

javeme Jul 8, 2020

zhoney Jul 8, 2020

zhoney Jul 8, 2020

Linary Jul 8, 2020

javeme commented Jul 8, 2020

zhoney Jul 9, 2020

zhoney Jul 9, 2020

javeme Jul 9, 2020

		@@ -242,17 +237,19 @@ public int pendingTasks() {
		}

		protected void notifyNewTask(HugeTask<?> task) {

		@@ -212,8 +273,8 @@ protected Id save(HugeServerInfo server) {
		});

		E.checkArgumentNotNull(task, "Task can't be null");
		this.checkOnMasterNode("schedule");

fix task scheduler bugs: dead lock and dup server-info #1079

fix task scheduler bugs: dead lock and dup server-info #1079

Conversation

javeme commented Jul 3, 2020 • edited

codecov bot commented Jul 7, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javeme commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javeme commented Jul 3, 2020 •

edited

codecov bot commented Jul 7, 2020 •

edited