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

termAt doesn't return Term.initial() for LogEntryIndex.initial() after ancestor is changed #110

Merged
merged 8 commits into from
Jan 11, 2022

Conversation

negokaz
Copy link
Contributor

@negokaz negokaz commented Dec 2, 2021

Fixes #105

Locations that uses termAt

publishAppendEntries

  private[this] def publishAppendEntries(): Unit = {
    resetHeartbeatTimeoutTimer()
    otherMemberIndexes.foreach { memberIndex =>
      val nextIndex    = currentData.nextIndexFor(memberIndex)
      val prevLogIndex = nextIndex.prev()
      val prevLogTerm  = currentData.replicatedLog.termAt(prevLogIndex)

akka-entity-replication/Leader.scala at v2.0.0 · lerna-stack/akka-entity-replication

affected part of #105

hasMatchLogEntry

  def hasMatchLogEntry(prevLogIndex: LogEntryIndex, prevLogTerm: Term): Boolean = {
    // リーダーにログが無い場合は LogEntryIndex.initial が送られてくる。
    // そのケースでは AppendEntries が成功したとみなしたいので、
    // prevLogIndex が LogEntryIndex.initial の場合はマッチするログが存在するとみなす
    prevLogIndex == LogEntryIndex.initial() || replicatedLog.termAt(prevLogIndex).contains(prevLogTerm)
  }

akka-entity-replication/RaftMemberData.scala at v2.0.0 · lerna-stack/akka-entity-replication

No impact, because termAt doesn't take prevLogIndex if it is LogEntryIndex.initial().

resolveSnapshotTargets

  def resolveSnapshotTargets(): (Term, LogEntryIndex, Set[NormalizedEntityId]) = {
    replicatedLog.termAt(lastApplied) match {
      case Some(lastAppliedTerm) =>
        (
          lastAppliedTerm,
          lastApplied,
          replicatedLog.sliceEntriesFromHead(lastApplied).flatMap(_.event.entityId.toSeq).toSet,
        )
      case None =>
        // This exception is not thrown unless there is a bug
        throw new IllegalStateException(s"Term not found at lastApplied: $lastApplied")
    }
  }

akka-entity-replication/RaftMemberData.scala at v2.0.0 · lerna-stack/akka-entity-replication

No impact, because resoveSnapshotTargets isn't call when lastApplied is LogEntryIndex.initial().
hasLogEntriesThatCanBeCompacted should return false in the situation.

    if (
      currentData.replicatedLog.entries.size >= settings.compactionLogSizeThreshold
      && currentData.hasLogEntriesThatCanBeCompacted
    ) {
      val (term, logEntryIndex, entityIds) = currentData.resolveSnapshotTargets

akka-entity-replication/RaftActor.scala at v2.0.0 · lerna-stack/akka-entity-replication

  def hasLogEntriesThatCanBeCompacted: Boolean = {
    replicatedLog.sliceEntriesFromHead(lastApplied).nonEmpty
  }

akka-entity-replication/RaftMemberData.scala at v2.0.0 · lerna-stack/akka-entity-replication

sliceEntriesFromHead passed following test:

      val logEntries = Seq(
        LogEntry(LogEntryIndex(1), EntityEvent(None, "a"), Term(1)),
        LogEntry(LogEntryIndex(2), EntityEvent(None, "b"), Term(1)),
        LogEntry(LogEntryIndex(3), EntityEvent(None, "c"), Term(1)),
      )

      val log = new ReplicatedLog(logEntries)

      log.sliceEntriesFromHead(to = LogEntryIndex.initial()) should be(empty)

@negokaz negokaz requested a review from tksugimoto December 3, 2021 03:55
@xirc xirc added this to the v2.1.0 milestone Dec 23, 2021
@xirc xirc requested review from xirc and removed request for xirc December 23, 2021 07:46
Copy link
Contributor

@xirc xirc left a comment

Choose a reason for hiding this comment

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

Great 👍 I left some comments.

Copy link
Contributor

@xirc xirc left a comment

Choose a reason for hiding this comment

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

Great! LGTM 👍

@tksugimoto tksugimoto merged commit f89e6c8 into master Jan 11, 2022
@tksugimoto tksugimoto deleted the fix-leader-breaks-follower-log branch January 11, 2022 10:47
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.

Starting a follower member later than leader completes a compaction may break ReplicatedLog of the follower
3 participants