Introduce Redis and Zookeeper locking modules. #1382

kishorebanala · 2019-11-08T02:36:24Z

https://netflix.github.io/conductor/technicaldetails/#maintaining-workflow-consistency-with-distributed-locking-and-fencing-tokens

coveralls · 2019-11-08T02:50:44Z

Pull Request Test Coverage Report for Build 3357

139 of 251 (55.38%) changed or added relevant lines in 15 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.3%) to 69.736%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core/src/main/java/com/netflix/conductor/core/utils/NoopLockModule.java	2	3	66.67%
redis-lock/src/main/java/com/netflix/conductor/locking/redis/config/RedisLockConfiguration.java	4	5	80.0%
core/src/main/java/com/netflix/conductor/core/config/Configuration.java	3	6	50.0%
core/src/main/java/com/netflix/conductor/metrics/Monitors.java	0	4	0.0%
redis-lock/src/main/java/com/netflix/conductor/locking/redis/config/RedisLockModule.java	0	4	0.0%
zookeeper-lock/src/main/java/com/netflix/conductor/zookeeper/config/ZookeeperModule.java	0	4	0.0%
core/src/main/java/com/netflix/conductor/core/utils/NoopLock.java	0	6	0.0%
core/src/main/java/com/netflix/conductor/core/execution/WorkflowExecutor.java	52	63	82.54%
server/src/main/java/com/netflix/conductor/bootstrap/ModulesProvider.java	4	18	22.22%
core/src/main/java/com/netflix/conductor/service/ExecutionLockService.java	15	31	48.39%

Totals
Change from base Build 3342:	-0.3%
Covered Lines:	10362
Relevant Lines:	14859

💛 - Coveralls

codecov · 2019-11-08T02:51:37Z

Codecov Report

Merging #1382 into dev will increase coverage by 0.05%.
The diff coverage is 49.4%.

@@             Coverage Diff              @@
##                dev    #1382      +/-   ##
============================================
+ Coverage     64.19%   64.25%   +0.05%     
- Complexity     2847     2927      +80     
============================================
  Files           241      251      +10     
  Lines         14354    14736     +382     
  Branches       1406     1458      +52     
============================================
+ Hits           9214     9468     +254     
- Misses         4366     4484     +118     
- Partials        774      784      +10

Impacted Files	Coverage Δ	Complexity Δ
.../com/netflix/conductor/core/config/CoreModule.java	`100% <ø> (ø)`	`21 <0> (ø)`	⬇️
...ix/conductor/zookeeper/config/ZookeeperModule.java	`0% <0%> (ø)`	`0 <0> (?)`
...n/java/com/netflix/conductor/metrics/Monitors.java	`69.37% <0%> (-1.5%)`	`40 <0> (+3)`
...onductor/locking/redis/config/RedisLockModule.java	`0% <0%> (ø)`	`0 <0> (?)`
...ava/com/netflix/conductor/core/utils/NoopLock.java	`0% <0%> (ø)`	`0 <0> (?)`
...config/SystemPropertiesRedisLockConfiguration.java	`100% <100%> (ø)`	`1 <1> (?)`
...uctor/zookeeper/config/ZookeeperConfiguration.java	`100% <100%> (ø)`	`3 <3> (?)`
...config/SystemPropertiesZookeeperConfiguration.java	`100% <100%> (ø)`	`1 <1> (?)`
...m/netflix/conductor/bootstrap/ModulesProvider.java	`42.3% <11.11%> (-9.36%)`	`4 <0> (ø)`
...etflix/conductor/service/ExecutionLockService.java	`38.7% <38.7%> (ø)`	`7 <7> (?)`
... and 20 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 27ab532...b25f608. Read the comment docs.

apanicker-nflx · 2019-11-09T00:58:44Z

core/src/main/java/com/netflix/conductor/core/config/NoopLockModule.java

@@ -0,0 +1,18 @@
+package com.netflix.conductor.core.config;


Maybe move this into a separate package.

apanicker-nflx · 2019-11-09T00:59:25Z

core/src/main/java/com/netflix/conductor/core/execution/WorkflowExecutor.java

@@ -49,11 +49,13 @@
 import com.netflix.conductor.core.execution.tasks.WorkflowSystemTask;
 import com.netflix.conductor.core.metadata.MetadataMapperService;
 import com.netflix.conductor.core.orchestration.ExecutionDAOFacade;
+import com.netflix.conductor.core.utils.ExternalPayloadStorageUtils;


Remove unused import.

apanicker-nflx · 2019-11-12T08:23:50Z

core/src/main/java/com/netflix/conductor/core/orchestration/ExecutionDAOFacade.java

@@ -193,6 +193,7 @@ public String createWorkflow(Workflow workflow) {
     * @param workflow the workflow tp be updated
     * @return the id of the updated workflow
     */
+    //TODO: include version in workflow object


Is this TODO to be resolved as part of this PR?

apanicker-nflx · 2019-11-12T08:24:47Z

core/src/main/java/com/netflix/conductor/core/utils/NoopLock.java

@@ -0,0 +1,24 @@
+package com.netflix.conductor.core.utils;


Add license

apanicker-nflx · 2019-11-12T08:26:16Z

core/src/main/java/com/netflix/conductor/core/utils/Lock.java

+    boolean acquireLock(String lockId, long timeToTry, TimeUnit unit);
+
+    /**
+     * acquires a re-entrant lock on lockId, blocks for timeToTry duration before giving up


Consider adding a description about usage of leaseTIme to the javadoc. The difference between this API and the one above is not clear.

Maybe rename the first one to waitForLock?

apanicker-nflx · 2019-11-12T08:49:29Z

zookeeper-lock/src/main/java/com/netflix/conductor/zookeeper/config/ZookeeperConfiguration.java

+import org.apache.curator.framework.CuratorFrameworkFactory;
+
+public interface ZookeeperConfiguration extends Configuration {
+    String ZK_CONNECTION = "zk.connection";


workflow.zookeeper.lock.connection

apanicker-nflx · 2019-11-12T08:49:47Z

zookeeper-lock/src/main/java/com/netflix/conductor/zookeeper/config/ZookeeperConfiguration.java

+
+public interface ZookeeperConfiguration extends Configuration {
+    String ZK_CONNECTION = "zk.connection";
+    String ZK_SESSIONTIMEOUT_MS = "zk.sessionTimeoutMs";


workflow.zookeeper.lock.session.timeout.ms

sessionTimeoutMs is a Zookeeper property we're passing through. Keeping it as workflow.zookeeper.lock.sessionTimeoutMs.

apanicker-nflx · 2019-11-12T08:50:07Z

zookeeper-lock/src/main/java/com/netflix/conductor/zookeeper/config/ZookeeperConfiguration.java

+public interface ZookeeperConfiguration extends Configuration {
+    String ZK_CONNECTION = "zk.connection";
+    String ZK_SESSIONTIMEOUT_MS = "zk.sessionTimeoutMs";
+    String ZK_CONNECTIONTIMEOUT_MS = "zk.connectionTimeoutMs";


workflow.zookeeper.lock.connection.timeout.ms

apanicker-nflx · 2019-11-12T08:50:36Z

zookeeper-lock/src/main/java/com/netflix/conductor/zookeeper/config/ZookeeperModule.java

@@ -0,0 +1,20 @@
+package com.netflix.conductor.zookeeper.config;


Add license

apanicker-nflx · 2019-11-12T08:50:48Z

zookeeper-lock/src/test/java/com/netflix/conductor/zookeeper/ZookeeperLockTest.java

@@ -0,0 +1,179 @@
+package com.netflix.conductor.zookeeper;


Add license

milongapau · 2019-11-12T21:08:41Z

core/src/main/java/com/netflix/conductor/core/config/Configuration.java

+
+    String IGNORE_LOCKING_EXCEPTIONS_PROPERTY_NAME = "locking.exceptions.ignore";
+    boolean IGNORE_LOCKING_EXCEPTIONS_DEFAULT_VALUE = false;
+
    //TODO add constants for input/output external payload related properties.


do you need to add this?

This helps to optionally ignore the locking exceptions (not the failures to obtain lock though) and not impact workflow executions.

apanicker-nflx · 2019-11-13T07:33:02Z

server/src/main/resources/server.properties

+workflow.decider.locking.enabled=false
+workflow.decider.locking.namespace=
+workflow.decider.locking.server=noop_lock
+workflow.decider.locking.leaseTimeInSeconds=60


Maybe rename to workflow.decider.locking.lease.time.seconds

apanicker-nflx

Please squash and merge.

* Added configurable timeouts for Zookeeper. * Rename config parameters.

…dress.

…ontinue execution.

Introduce Redis and Zookeeper locking modules.

kishorebanala requested review from apanicker-nflx and a user November 8, 2019 02:36

kishorebanala requested a review from milongapau November 9, 2019 00:51

apanicker-nflx reviewed Nov 12, 2019

View reviewed changes

milongapau reviewed Nov 12, 2019

View reviewed changes

apanicker-nflx reviewed Nov 13, 2019

View reviewed changes

apanicker-nflx approved these changes Nov 13, 2019

View reviewed changes

apau and others added 15 commits November 13, 2019 10:33

defines locking API with a no-op implementation.

ee6d868

Lock implementation using Zookeeper

9425b32

fix javadoc error

6a9b917

fix Guice binding issue

502126a

debugging, will clean up later.

020466c

clean up zknodes when wf completes.

b80a246

Release lock before delete on completeWorkflow

bb961df

Configurable zk timeouts (#1312)

77b7199

* Added configurable timeouts for Zookeeper. * Rename config parameters.

Changing HttpTask test URI to fix failing test with conflicting IP Ad…

69852d1

…dress.

Change to ZK nodes guaranteed delete instead of background delete.

42c3158

Fix tests for Workflow executor changes from locking service.

14723e6

Disable decider service locking by default.

3083fe9

Implementing workflow decider locking with Redis.

90ab659

Updated Redisson cluster mode config init.

952953e

Adding configuration to optionally ignore lock acquire failures and c…

b25f608

…ontinue execution.

kishorebanala force-pushed the feature/redis-lock branch from bb643f7 to b25f608 Compare November 13, 2019 18:35

kishorebanala merged commit bfe3007 into dev Nov 13, 2019

apanicker-nflx deleted the feature/redis-lock branch December 10, 2019 22:00

peterlau pushed a commit that referenced this pull request Sep 1, 2021

Merge pull request #1382 from Netflix/feature/redis-lock

0368665

Introduce Redis and Zookeeper locking modules.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Redis and Zookeeper locking modules. #1382

Introduce Redis and Zookeeper locking modules. #1382

kishorebanala commented Nov 8, 2019 •

edited

Loading

coveralls commented Nov 8, 2019 •

edited

Loading

codecov bot commented Nov 8, 2019 •

edited

Loading

apanicker-nflx Nov 9, 2019

apanicker-nflx Nov 9, 2019

apanicker-nflx Nov 12, 2019

apanicker-nflx Nov 12, 2019

apanicker-nflx Nov 12, 2019

milongapau Nov 12, 2019

apanicker-nflx Nov 12, 2019

apanicker-nflx Nov 12, 2019

kishorebanala Nov 13, 2019

apanicker-nflx Nov 12, 2019

apanicker-nflx Nov 12, 2019

apanicker-nflx Nov 12, 2019

milongapau Nov 12, 2019

kishorebanala Nov 13, 2019

apanicker-nflx Nov 13, 2019

apanicker-nflx left a comment

		@@ -0,0 +1,20 @@
		package com.netflix.conductor.zookeeper.config;

Introduce Redis and Zookeeper locking modules. #1382

Introduce Redis and Zookeeper locking modules. #1382

Conversation

kishorebanala commented Nov 8, 2019 • edited Loading

coveralls commented Nov 8, 2019 • edited Loading

Pull Request Test Coverage Report for Build 3357

💛 - Coveralls

codecov bot commented Nov 8, 2019 • edited Loading

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

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

apanicker-nflx left a comment

Choose a reason for hiding this comment

kishorebanala commented Nov 8, 2019 •

edited

Loading

coveralls commented Nov 8, 2019 •

edited

Loading

codecov bot commented Nov 8, 2019 •

edited

Loading