Skip to content
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.

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

track lowest allowed timestamp per persistence ID #110

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leviramsey
Copy link
Contributor

@leviramsey leviramsey commented Nov 29, 2024

References #108

Socializing this before writing tests, but the idea is:

  • on replay, record the minimum acceptable next timestamp for this pid
  • on write, check the desired timestamp (from InstantFactory.now()) against the acceptable timestamp, choose the later of the two. If the later one is the minimum acceptable, then update the minimum acceptable for next time.
  • periodically, the minimum acceptable next timestamps are checked against InstantFactory.now() (local monotonic clock): earlier ones are evicted because monotonicity ensures that the next write will be after the minimum.

Net performance impact should be minimal:

  • in a recovery, we'll only write for the last event: this write is done as a local ask: it would need a high level of contention to add noticeable latency to recoveries
  • when writing events (more common), the check is done against a ConcurrentHashMap view, so should be fast. In the unlikely event we need to use a late timestamp than we want (viz. we've detected clock skew), there will be a local ask: this is like the recovery case, but happens more often, so it might have an impact but this should be "transitory": the minimum acceptable timestamp will advance by a microsecond per event, and it's unreasonable to expect thousands of events for a persistence ID per second (so InstantFactory.now() should be moving a few orders of magnitude faster than the minimum acceptable)
  • contention for updates to minimum acceptable timestamp is ameliorated by partitioning by persistence plugin and slice range (with a number of slice ranges at least the detected number of CPUs)

Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

Could be useful to start with just some detection, while this is under development and discussion.

import java.util.concurrent.ConcurrentHashMap
import java.time.temporal.ChronoUnit

object MonotonicTimestamps extends ExtensionId[MonotonicTimestamps] {
Copy link
Contributor

Choose a reason for hiding this comment

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

May just be me, but the implementation looks more complex than what we need. Feel that it could be simpler, but will think through it some more too.

The journal is an actor, so could also track directly. Also see how writesInProgress are tracked in the journal implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Haven't looked at the details here. Would it be enough to increase the time in InstantFactory when detecting clock skew? At the point the warning is logged in #110

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, was thinking the same. We could just bump in InstantFactory. That will then be monotonically increasing micros until the current time catches up and it reverts to regular timestamps again. And probably check against a configurable tolerance setting, so it can only be skewed by so much, otherwise error.

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.

3 participants