GH-66: Add DynamoDbLockRegistry implementation #93

artembilan · 2018-05-24T20:00:12Z

Fixes #66

artembilan · 2018-05-24T20:01:32Z

Depends on: spring-projects/spring-integration#2455

would you mind, please, to take a look here?

I don't tell that you need to jump into the AWS details, just wonder if you could find some flaws around Java concurrency usage.

Thanks

artembilan · 2018-05-24T20:28:30Z

BTW, need Docs on the matter and maybe more JavaDocs (depends of the review).
Will come back to that after some feedback.

Thanks

garyrussell

That's all for this pass.

garyrussell · 2018-05-24T20:19:29Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+
+	@Override
+	public boolean isAutoStartup() {
+		return true;


Why not configurable?

If you take a look into the start(), there is nothing just table creation on the AWS: I just moved the hard network job into the start() instead of afterPropertiesSet().
Therefore I don't see reason to have this not started automatically - it isn't going to be useful without created table. Or... the table must be provided in advance.

I can't reconsider this if there is really a strong feeling on the matter.

By the way: is it going to work just with the Lifecycle? I mean this will be as a bean and container will start it anyway. The point is do not expose lifecycle control to end user at all.

Thanks

Another inconvenience that this AmazonDynamoDBLockClient is not async and we really can't initiate request and exit, like we do in the DynamoDbMetaDataStore.afterPropertiesSet().

OK; thanks for the explanation; no Lifecycle beans are not started unless the user start()s the container.

OK! I decided to remove lifecycle control in favor of the separate thread execution in the afterPropertiesSet().
See an update in commits.

garyrussell · 2018-05-24T20:21:31Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+					// This is allowed minimum for constant AWS requests.
+					Thread.sleep(1000);
+				}
+				catch (InterruptedException e) {


Thread.currentThread().interrupt().

garyrussell · 2018-05-24T20:22:12Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+
+	@Override
+	public void stop() {
+		this.running.set(false);


Nothing to do on AWS?

Well, agreed. Will move the destroy() logic here. When we stopped, we shouldn't schedule any heart beats to the AWS. Good catch! 👍

garyrussell · 2018-05-24T20:22:36Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+
+	@Override
+	public boolean isRunning() {
+		return false;


this.running.get()

garyrussell · 2018-05-24T20:23:13Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+
+	@Override
+	public int getPhase() {
+		return Integer.MAX_VALUE;


Why not configurable? Shouldn't it be relatively early?

M-m-m. I thought Integer.MAX_VALUE is the last phase in the order. No?
Should it be Integer.MIN_VALUE then.

On the other hand it really might be better to create table on AWS as early as possible and let our consumers (e.g. aggregator) to do its work just in time.

Max value seems wrong in cases where someone wants to get a lock in a Lifecycle.start().

I would have thought earlier than max_value, but certainly not min-value; maybe 0 is the best in this case.

garyrussell · 2018-05-24T20:24:13Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+		}
+		catch (InterruptedException e) {
+			Thread.currentThread().interrupt();
+			throw new IllegalStateException("The DynamoDb table " + this.tableName +


Shouldn't this be also thrown in the await returns false?

garyrussell · 2018-05-24T20:25:47Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+
+	@Override
+	public Lock obtain(Object lockKey) {
+		Assert.isInstanceOf(String.class, lockKey);


Deprecated assertion - needs message.

garyrussell · 2018-05-24T20:27:03Z

src/main/java/org/springframework/integration/aws/lock/DynamoDbLockRegistry.java

+				 * This method must be uninterruptible so catch and ignore
+				 * interrupts and only break out of the while loop when
+				 * we get the lock.
+				 */


We should reinstate the interrupt before exiting, though - set a boolean and add finally to an outer try?

That's just Dave Syer's magic copy/paste from the JdbcLockRegistry 😄 . Sorry, I don't understand what you mean.
Some code sample?

public void lock() { this.delegate.lock(); boolean wasInterruptedWhileUninterruptible = false; try { while (true) { try { while (!doLock()) { Thread.sleep(100); //NOSONAR } break; } catch (CannotSerializeTransactionException | TransactionTimedOutException | QueryTimeoutException e) { // try again } catch (InterruptedException e) { /* * This method must be uninterruptible so catch and ignore * interrupts and only break out of the while loop when * we get the lock. */ wasInterruptedWhileUninterruptible = true; } catch (Exception e) { this.delegate.unlock(); rethrowAsLockException(e); } } } finally { if (wasInterruptedWhileUninterruptible) { Thread.currentThread().interrupt(); } } }

execution in the `afterPropertiesSet()` * Fix `lock()` interruptibility logic

garyrussell · 2018-05-30T19:25:02Z

build.gradle

@@ -23,6 +23,7 @@ apply from: "${rootProject.projectDir}/publish-maven.gradle"
 group = 'org.springframework.integration'

 repositories {
+	mavenLocal()


Intentional?

Right, because I tested it against the build in my just merged PR in SI spring-projects/spring-integration#2455.

Now this change can be reverted.

* decrease an amount of expectations in the Kinesis test

spring-projectsGH-66: Add DynamoDbLockRegistry implementation

3b80fce

Fixes spring-projects#66

artembilan mentioned this pull request May 24, 2018

Checkpointing logic resulting in lost messages in a clustered environment (M2 release) #90

Closed

garyrussell suggested changes May 24, 2018

View reviewed changes

* Remove Lifecycle from DynamoDbLockRegistry in favor of a thread

0aeb60c

execution in the `afterPropertiesSet()` * Fix `lock()` interruptibility logic

garyrussell suggested changes May 30, 2018

View reviewed changes

artembilan added 3 commits May 30, 2018 16:08

* Remove mavenLocal() since the upstream PR is merged

81a3efd

* decrease an amount of expectations in the Kinesis test

* Upgrade to SC-AWS-2.0.0.RC2

3903581

* Add Docs for the DynamoDbLockRegistry

29b727e

garyrussell merged commit 5c262f1 into spring-projects:master Jun 8, 2018

GH-66: Add DynamoDbLockRegistry implementation #93

GH-66: Add DynamoDbLockRegistry implementation #93

Uh oh!

Conversation

artembilan commented May 24, 2018

Uh oh!

artembilan commented May 24, 2018

Uh oh!

artembilan commented May 24, 2018

Uh oh!

garyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!