Skip to content

Conversation

shetsa-amzn
Copy link
Collaborator

Issue #, if available: #44

Description of changes:
The change in this PR after resolving few conflicts

#79

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@shetsa-amzn shetsa-amzn requested review from frubinst and pasoevi May 22, 2023 13:20
@shetsa-amzn shetsa-amzn self-assigned this May 22, 2023
@shetsa-amzn shetsa-amzn linked an issue May 22, 2023 that may be closed by this pull request
*/
public long millisecondTime() {
return System.nanoTime() / 1000000;
return System.currentTimeMillis();
Copy link

Choose a reason for hiding this comment

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

Just to be clear this PR doesn't account for clock skew nor monotonicity and introduces correctness bugs around those concepts

Copy link

Choose a reason for hiding this comment

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

Isn't updating this pre-existing method outside the scope of this CR? Unless I am missing something after reading https://stackoverflow.com/questions/351565/system-currenttimemillis-vs-system-nanotime this looks like an enhancement.

Copy link

@tso tso Jun 14, 2023

Choose a reason for hiding this comment

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

Updating this pre-existing method is required for the idea behind this PR, which is to use wall clock time to determine if a client can acquire the lock.

currentTimeMillis is wall clock time (can be converted into a time like 2023-06-13 8:37 PM)
nanoTime is an entirely random number that is only useful to measure elapsed time

I would propose #88 as a more nuanced PR as it:

  1. does not affect blocking clients by changing this method which is critical for correctness
  2. surfaces clock skew to the end user as a problem they need to consider

Copy link

Choose a reason for hiding this comment

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

That makes sense. Thank you for explaining.

Copy link

Choose a reason for hiding this comment

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

I agree that we should abandon this proposed change for #88 after it is revised.

@rahulvishwanatham
Copy link

I am facing the same issue with 1.2.0 version, is this going to be merged or any other solution to this? A lock is stale in my DDB and no other instance is able to acquire the lock. I am setting withShouldSkipBlockingWait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LockCurrentlyNotAvailable with shouldSkipBlockingWait

5 participants