Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Revert revert 5131 #5182

Merged
merged 4 commits into from
Jan 8, 2021
Merged

Revert revert 5131 #5182

merged 4 commits into from
Jan 8, 2021

Conversation

sudiksha27
Copy link
Contributor

@sudiksha27 sudiksha27 commented Jan 7, 2021

Goals (and why):
#5131 was reverted in #5181 as there were false positives. in #5131, it was (incorrectly) assumed the timestamps are strictly increasing with increasing sequence numbers. There is a possibility where consecutive seq numbers have the same timestamp. This is possible when a node gains leadership and does not know the value for latest sequence number - n . In this case, it forces agreement for nth sequence on the same value as that of (n-1)th or (n-2)th round (if value for n-1 is not known).
See PaxosTimestampBoundStore#getAgreedState for reference.

Implementation Description (bullets):
The invariant is now modified to - timestamps should not decrease with increasing sequence numbers.

Testing (What was existing testing like? What have you done to improve it?):
Added unit tests.
Also, I have validated the invariant on paxos logs from internal dev stack

Concerns (what feedback would you like?):
Corner cases I might have missed?

Where should we start reviewing?:
All of the code is same as in #5131 except the inequality check in LocalTimestampInvariantsVerifier#timestampInvariantsViolationLevel

Priority (whenever / two weeks / yesterday):
EOD tomorrow

@changelog-app
Copy link

changelog-app bot commented Jan 7, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

TimeLock now validates the expected invariant for its timestamp bounds i.e. the timestamp bound must not decrease with increasing sequence numbers.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@gmaretic gmaretic left a comment

Choose a reason for hiding this comment

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

Looks good!

@sudiksha27
Copy link
Contributor Author

Merge #5183 before getting this in.

@bulldozer-bot bulldozer-bot bot merged commit 00eb359 into develop Jan 8, 2021
@bulldozer-bot bulldozer-bot bot deleted the revert-revert-5131 branch January 8, 2021 15:36
@svc-autorelease
Copy link
Collaborator

Released 0.283.1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants