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.

Sign up for GitHub

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

Jump to bottom

Add coordinator durability #2029

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Add coordinator durability #2029

wants to merge 29 commits into from

Conversation

antoniofilipovic
Copy link
Collaborator

@antoniofilipovic antoniofilipovic commented May 11, 2024 •
edited
Loading

This diff adds durability to the coordinator log store and snapshots.

General description:

  • Log store and snapshots are stored under the same RocksDB instance
  • Log store and snapshots have versioning in place
  • Log store and snapshots are serialized and stored to disk
  • State manager is upgraded with versioning.
  • State manager updated with storing log_id, prev_log_id, and async_replication details
  • State manager also stores voted_for and term
  • Flag to enable storing everything to disk
  • ManagementServerConfig uses endpoint instead of IP address

@antoniofilipovic antoniofilipovic added the Docs needed Docs needed label May 11, 2024
@antoniofilipovic antoniofilipovic added this to the mg-v2.17.0 milestone May 11, 2024
@antoniofilipovic antoniofilipovic self-assigned this May 11, 2024
@gitbuda gitbuda modified the milestones: mg-v2.17.0, mg-v2.18.0 May 20, 2024
@as51340
Copy link
Contributor

as51340 commented May 26, 2024

I was reading something about Raft @antoniofilipovic. We should make logs, votedFor and logTerm durable. On every change of this property, you should persist these things to disk.
Probably you know all of this atm but I though maybe it will help
Reference

@antoniofilipovic
Copy link
Collaborator Author

I was reading something about Raft @antoniofilipovic. We should make logs, votedFor and logTerm durable. On every change of this property, you should persist these things to disk. Probably you know all of this atm but I though maybe it will help Reference

Yes, that is also implemented now, as part of state manager 👍

@antoniofilipovic antoniofilipovic force-pushed the add-durability-for-coord-logs-snapshot branch from 0fb9c04 to feaff28 Compare June 5, 2024 15:16
@antoniofilipovic antoniofilipovic marked this pull request as ready for review June 5, 2024 15:17
@antoniofilipovic antoniofilipovic added CI -build=coverage -test=core Run coverage build and core tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push feature feature labels Jun 5, 2024
as51340
as51340 reviewed Jun 6, 2024
View reviewed changes
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

Great work!
I would spend time improving the code since it is quite important IMO

src/coordination/coordinator_cluster_state.cpp Outdated Show resolved Hide resolved
src/coordination/include/nuraft/coordinator_cluster_state.hpp Outdated Show resolved Hide resolved
src/coordination/include/coordination/coordinator_communication_config.hpp Outdated Show resolved Hide resolved
src/coordination/include/coordination/coordinator_communication_config.hpp Show resolved Hide resolved
src/coordination/include/nuraft/coordinator_log_store.hpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
tests/e2e/high_availability/distributed_coords.py Outdated Show resolved Hide resolved
@antoniofilipovic antoniofilipovic force-pushed the add-durability-for-coord-logs-snapshot branch from ac9a91c to f1a9426 Compare June 7, 2024 16:19
@kgolubic
Copy link
Contributor

@antoniofilipovic I'm missing the documentation PR.

@antoniofilipovic
Copy link
Collaborator Author

Description

This diff adds durability to the coordinator log store and snapshots.

General description:

  • Log store and snapshots are stored under the same RocksDB instance ✔️
  • Log store and snapshots have versioning in place ✔️
  • Log store and snapshots are serialized and stored to disk ✔️
  • State manager is upgraded with versioning. ✔️
  • State manager updated with storing log_id, prev_log_id, and async_replication details ✔️
  • State manager also stores voted_for and term ✔️
  • Flag to enable storing everything to disk ✔️
  • ManagementServerConfig uses endpoint instead of IP address (@as51340 ) ✔️

[master < Task] PR

  • Provide the full content or a guide for the final git message
    -Add durability for the coordinator log store and snapshots

CI Testing Labels

Please select the appropriate CI test labels (CI -build=build-name -test=test-suite)

Documentation checklist

  • Add the documentation label tag
  • Add the bug / feature label tag
  • Add the milestone for which this feature is intended
    • If not known, set for a later milestone
  • Write a release note, including added/changed clauses
    • Coordinator now uses durability for log store and snapshots
  • Link the documentation PR here
    • [Documentation PR link]
  • Tag someone from docs team in the comments

as51340
as51340 reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

great work, have some smaller comments. In general, just make sure all behaviors are documented, it will be easier for us to handle this and for users

src/coordination/coordinator_instance.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/coordinator_log_store.cpp Outdated Show resolved Hide resolved
src/coordination/utils.cpp Show resolved Hide resolved
src/flags/coord_flag_env_handler.cpp Outdated Show resolved Hide resolved
src/flags/coordination.cpp Show resolved Hide resolved
src/memgraph.cpp Outdated Show resolved Hide resolved
tests/e2e/high_availability/distributed_coords.py Show resolved Hide resolved
@antoniofilipovic antoniofilipovic force-pushed the add-durability-for-coord-logs-snapshot branch from f8153f2 to 21e5e10 Compare June 20, 2024 09:59
@as51340 as51340 self-requested a review June 20, 2024 12:45
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 20, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@as51340 as51340 as51340 approved these changes

Assignees

@antoniofilipovic antoniofilipovic

Labels
CI -build=coverage -test=core Run coverage build and core tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push Docs needed Docs needed feature feature
Projects
None yet
Milestone
mg-v2.18.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@antoniofilipovic @as51340 @kgolubic @gitbuda