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

feat(v2): implement ReadIndex for linearizable reads #3619

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

marcsanmi
Copy link
Contributor

This PR implements the ReadIndex in the Metastore to ensure linearizable reads, as described in the Raft consensus algorithm. The implementation leverages the Hashicorp Raft library's existing features while adding necessary checks to guarantee read consistency.

Key changes:

  1. Update ReadIndex method to verify leadership and return current commit index.
  2. Added readIndex method to ensure the node is up-to-date before serving reads.
  3. Modified QueryMetadata and GetProfileStats to use readIndex before querying data.
  4. Updated CheckReady method to use readIndex for consistency checks.

Raft Algorithm Considerations:

  • Hashicorp's Raft implementation already sends a no-op log entry when runLeader is called for the leader to be up to the latest commit index, which aligns with the Raft paper's recommendation for ensuring a committed entry from the current term (thanks @kolesnikovae for the heads up).

Testing Considerations:

  • Unit tests were not added for these changes as they primarily interact with the Raft package itself.
  • Integration tests would be more suitable for verifying the correct interaction with Raft and ensuring linearizable reads.
  • The current metastore lacks integration tests, so adding them would be a significant change that deserves its own focused effort.

@marcsanmi marcsanmi force-pushed the marcsanmi/enhance-use-read-index branch from fc05998 to 910451b Compare October 10, 2024 10:58
@marcsanmi marcsanmi force-pushed the marcsanmi/enhance-use-read-index branch from 3a1a54b to 17673e1 Compare October 14, 2024 07:15
@marcsanmi marcsanmi marked this pull request as ready for review October 14, 2024 07:16
@marcsanmi marcsanmi requested a review from a team as a code owner October 14, 2024 07:16
Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this @marcsanmi!

@korniltsev could you please take a look as well? Just to make 100% sure the approach is valid

We might need it for deployments outside k8s
@kolesnikovae kolesnikovae force-pushed the marcsanmi/enhance-use-read-index branch from a2b5e4e to 49c6249 Compare October 15, 2024 05:02
@kolesnikovae
Copy link
Collaborator

I've reverted the min ready check:

  1. This is not quite the same as k8s minReadySeconds
  2. It might be useful in deployments outside of k8s

@kolesnikovae kolesnikovae enabled auto-merge (squash) October 15, 2024 05:27
@kolesnikovae kolesnikovae merged commit 4aa3bfc into main Oct 15, 2024
18 checks passed
@kolesnikovae kolesnikovae deleted the marcsanmi/enhance-use-read-index branch October 15, 2024 05:31
Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

LGTM

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