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

tracker: track in-flight commit index #171

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Feb 26, 2024

This commit adds a Progress.sentCommit field tracking the highest commit index which the leader sent to the follower. It is used to distinguish cases when a commit index update needs or doesn't need to be sent to a follower.

Touches #131

@pav-kv pav-kv requested a review from ahrtr February 26, 2024 16:18
@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 26, 2024

@ahrtr PTAL. Forked this from #134. This reduces the amount of unnecessary empty MsgApp messages, see the effect on the tests in testdata.

@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 26, 2024

@serathius Can I ask you to review this too? CRDB reviewers are OOO this week.

@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 26, 2024

@bdarnell This addresses your comment to a good degree, without changing the protocol.

tracker/progress.go Outdated Show resolved Hide resolved
@pav-kv pav-kv force-pushed the track-inflight-commit branch 2 times, most recently from 02ac83b to 0611757 Compare February 27, 2024 18:55
@pav-kv pav-kv requested a review from ahrtr February 27, 2024 18:58
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 28, 2024

@serathius Could you take a look please?

@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 28, 2024

@ahrtr Are you expecting a second review? Can this be merged? Alternatively, could you start reviewing #134 that's built on top of this PR (just ignore the first two commits which are this PR)?

@ahrtr
Copy link
Member

ahrtr commented Feb 28, 2024

@ahrtr Are you expecting a second review?

Yes, it's definitely better to have a second review for any behaviour or logic change. We need to minimize any risk as much as possible.

For any mechanical change, such as just renaming or simple code refactor, it's OK to merge with only one approval. Such as #172

could you start reviewing #134

It's in my to-do list. Will take a look later.

tracker/progress.go Outdated Show resolved Hide resolved
@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 29, 2024

@serathius ping

@pav-kv pav-kv removed the request for review from serathius March 1, 2024 13:30
@pav-kv
Copy link
Contributor Author

pav-kv commented Mar 1, 2024

@ptabor @spzala Could you review/approve please?

@ahrtr ahrtr requested a review from erikgrinaker March 4, 2024 10:27
tracker/progress.go Outdated Show resolved Hide resolved
tracker/progress.go Outdated Show resolved Hide resolved
@pav-kv pav-kv force-pushed the track-inflight-commit branch 2 times, most recently from 7e87703 to 0611757 Compare March 4, 2024 15:55
@pav-kv pav-kv requested a review from erikgrinaker March 5, 2024 13:45
tracker/progress.go Outdated Show resolved Hide resolved
tracker/progress.go Show resolved Hide resolved
tracker/progress.go Outdated Show resolved Hide resolved
@pav-kv
Copy link
Contributor Author

pav-kv commented Mar 6, 2024

@ahrtr All actionable comments in this PR are resolved, and the open discussion motivates a second PR #132 after this one. This PR is ready for merge.

@pav-kv pav-kv requested a review from erikgrinaker March 6, 2024 14:28
@ahrtr
Copy link
Member

ahrtr commented Mar 7, 2024

Please also rebase this PR to resolve the workflow failure.

When we send a snapshot, this is equivalent to sending all entries up to
the snapshot's index. Correspondingly, we update the Next index to
reflect this in-flight state.

Signed-off-by: Pavel Kalinnikov <[email protected]>
This commit adds a Progress.pendingCommit field tracking the highest
commit index <= Next-1 which the leader sent to the follower. It is used
to distinguish cases when a commit index update needs or doesn't need to
be sent to a follower.

Signed-off-by: Pavel Kalinnikov <[email protected]>
@pav-kv
Copy link
Contributor Author

pav-kv commented Mar 7, 2024

@ahrtr done

@ahrtr ahrtr merged commit bc285dd into etcd-io:main Mar 7, 2024
10 checks passed
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.

4 participants