Skip to content

GH-3272: Support lease renewal for distributed locks #3317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

GH-3272: Support lease renewal for distributed locks #3317

wants to merge 1 commit into from

Conversation

astrubel
Copy link
Contributor

@astrubel astrubel commented Jul 1, 2020

Lock TTL renewal feature for JDBC

issue #3272

I retrieved some lines from stefanvassilev@cb5d968

artembilan
artembilan requested changes Jul 1, 2020
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, run gradlew check locally before pushing commits:

[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java:289:33: '}' at column 5 should be alone on a line. [RightCurly]

[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java:291:33: '}' at column 5 should be alone on a line. [RightCurly]

[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/RenewableLockRegistry.java:21:1: Wrong order for 'java.util.concurrent.locks.Lock' import. [ImportOrder]

Thanks.

I wonder also what do you think about other LockRegistry implementations...

@astrubel astrubel force-pushed the jdbc-lock-renewal branch from 544924e to a8b5d54 Compare July 1, 2020 14:27
@astrubel
Copy link
Contributor Author

astrubel commented Jul 1, 2020

@artembilan I'm sorry for the code style committed...

For the other implementations I can merge my new RenewableLockRegistry interface with the existing ExpirableLockRegistry one but I don't know how to implement renewLock() for them..

@artembilan
Copy link
Member

You are right.
Let's review your current effort and think about all others later.
Probably your idea with RenewableLockRegistry is correct and all the lock registries could support it.
For example new FencedLock in Hazelcast based on Raft Consensus Algorithm doesn't not provide any renewal hook, just because everything is based on the session heartbeat. And so on.

I'll give you some feedback to your code a bit later today.

artembilan
artembilan requested changes Jul 1, 2020
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all I thought we would need to introduce a DistributedLock abstraction and have that renew over there, but then I have figured that we may not have access to lock object in some places. More over it turns out that not all distributed locks (Hazelcast, Zookeer, Geode etc) give us access to some renewal hook.
Therefore it really should be a particular LockRegistry contract like you did with this RenewableLockRegistry.

@garyrussell , WDYT?

We need to add some docs for this new feature in the jdbc.adoc and a note with the link in the whats-new.adoc.

@garyrussell
Copy link
Contributor

RenewableLockRegistry is fine; but I think it should move to core instead of being in the jdbc module.

@astrubel astrubel force-pushed the jdbc-lock-renewal branch from a8b5d54 to efffe4b Compare July 2, 2020 14:26
artembilan
artembilan requested changes Jul 2, 2020
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to add a whats-new.adoc not for that RenewableLockRegistry and explain its behavior in the jdbc.adoc at the jdbc-lock-registry section?

When we merge this, let's consider which other LockRegistry implementations could support renewing as well.

Thanks

artembilan
artembilan requested changes Jul 2, 2020
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to take a look why JDBC lock tests are failing:

JdbcLockRegistryDifferentClientTests > testRenewLock FAILED

    java.lang.IllegalArgumentException at JdbcLockRegistryDifferentClientTests.java:377

JdbcLockRegistryTests > testLockRenewLockNotOwned FAILED

    org.opentest4j.AssertionFailedError at JdbcLockRegistryTests.java:283

        Caused by: java.lang.IllegalArgumentException at JdbcLockRegistryTests.java:283

JdbcLockRegistryTests > testLockRenew FAILED

    java.lang.IllegalArgumentException at JdbcLockRegistryTests.java:276

?

Looks like some of them are new yours...

@astrubel astrubel force-pushed the jdbc-lock-renewal branch from efffe4b to d4786ff Compare July 3, 2020 10:14
@astrubel
Copy link
Contributor Author

astrubel commented Jul 3, 2020

There are 3 others test failed now.. :(

RotatingServersTests > testStreaming() FAILED

    java.lang.AssertionError at RotatingServersTests.java:193

RotatingServersTests > testVariableLocalDir() FAILED

    java.lang.AssertionError at RotatingServersTests.java:174

RotatingServersTests > testStandard() FAILED

    java.lang.AssertionError at RotatingServersTests.java:138

artembilan
artembilan requested changes Jul 6, 2020
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple nit-picks.

Thanks

...tion-jdbc/src/test/java/org/springframework/integration/jdbc/lock/JdbcLockRegistryTests.java
public void testLockRenewLockNotOwned() {
this.registry.obtain("foo");

Assertions.assertThrows(IllegalMonitorStateException.class, () -> registry.renewLock("foo"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer AssertJ assertions instead: assertThatExceptionOfType()

src/reference/asciidoc/jdbc.adoc
Starting with version 5.4, the `RenewableLockRegistry` interface has been introduced and added to `JdbcLockRegistry`.
The `renewLock()` method must be called during locked process in case of the locked process would be longer than time to live of the lock.
So the time to live can be highly reduce and deployments can retake a lost lock quickly.
NB. The lock renewal can be done only if the lock is held by the current thread, so the locked process has to be executed in another thread.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is this NB about.
Can we have a more official language in the docs, please?

Copy link
Contributor

@garyrussell garyrussell Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB (latin Nota Bene - "note well") is commonly used in real (British) English - it is not common in the USA.

Use AsciiDoctor NOTE: instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right! Didn't think through. This sentence is really NOTE:.

Thank you, Gary!

OK. So, I'm pulling this locally and fixing the latest simple concerns on merge.

@artembilan
Copy link
Member

Merged as e2c6e77 after some code style and language fixes.

@astrubel ,

thank you very much for the contribution; looking forward for more!

@artembilan artembilan added this to the 5.4 M2 milestone Jul 6, 2020
@artembilan artembilan closed this Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell left review comments

@artembilan artembilan artembilan requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.4 M2
Development

Successfully merging this pull request may close these issues.
3 participants
@astrubel @artembilan @garyrussell