Skip to content

Conversation

@szetszwo
Copy link
Contributor

See RATIS-2350.

Copy link
Member

@SzyWilliam SzyWilliam left a comment

Choose a reason for hiding this comment

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

Beautiful work! Feels like reading math essay here.
Looks good to me excepts for a small question ;)

Comment on lines -108 to -110
if (current > lastApplied) {
readIndexQueue.complete(current);
}
Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember this double check is to prevent race condition, see #958. We may still face a race condition here because ReadIndexQueue.add is not totally synchronized. How about moving the entire ReadIndexQueue.add to synchronzied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SzyWilliam , thanks for reminding me about RATIS-1927!

In this pr, the check is moved to a synchronized block in ReadIndexQueue.add(..) and lastAppliedIndex is also updated synchronously. So, it won't have a race condition anymore.

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

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

LGTM

I see that linearizable is the strongest consistency level. Write-after-read consistency is somewhat weaker. For example, if client A first writes at index 1, and then client B writes at index 2, when client A queries the data, for linearizable, it must wait until the state machine has applied index 2 before it can query and get the correct result. However, for write-after-read consistency, the state machine only needs to apply up to index 1, and it can return the result immediately.

@szetszwo szetszwo removed the request for review from adoroszlai November 13, 2025 16:50
@szetszwo
Copy link
Contributor Author

@SzyWilliam , please see if you would take a second look. Otherwise, let's merge it tomorrow.

@szetszwo szetszwo merged commit 81c714d into apache:master Nov 14, 2025
15 checks passed
@szetszwo
Copy link
Contributor Author

@SzyWilliam , @OneSizeFitsQuorum , thanks for reviewing this!

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