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

Sync active memtable and value log on Db.Sync (#1847) #1953

Merged
merged 5 commits into from
Jun 3, 2023

Conversation

SarthakMakhija
Copy link
Contributor

@SarthakMakhija SarthakMakhija commented May 22, 2023

Fixes #1847

Currently, DB.Sync() only syncs the value log but not the WAL of the active memtable, however recovery happens from the WAL of the active memtable as a part of the DB.Open() method.

This change attempts to sync both the logs, however there can be an issue in syncing one of the logs. This change lists all of the possible cases that can arise during the sync operations. Some of these issues will be taken separately. For example,
the issue #1954 can arise and shall be handled separately.

This change adds a few tests to ensure that the Sync behavior is not broken. This change also adds a learning test (db2_test.go -> TestAssertValueLogIsNotWrittenToOnStartup) to ensure that value log is only read from (and not written to) during startup. This learning shall be used in the next issue (the one described above (*))

@CLAassistant
Copy link

CLAassistant commented May 22, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

I just realized that we may need to consider acquiring some lock while accessing the memtable and vlog. Please look into it. Also, address the comments from the CI. CI also is not happy.

db.go Show resolved Hide resolved
db.go Outdated Show resolved Hide resolved
db.go Outdated Show resolved Hide resolved
@SarthakMakhija
Copy link
Contributor Author

I just realized that we may need to consider acquiring some lock while accessing the memtable and vlog. Please look into it. Also, address the comments from the CI. CI also is not happy.

Linting part is ok, I will reduce the line length in the comment.
Pipeline is failing because of the coverage

FAIL
coverage: 64.8% of statements

I am not sure if I can cover this part.

@mangalaman93
Copy link
Contributor

I am not sure if I can cover this part.

Don't worry about it for now.

@mangalaman93 mangalaman93 self-requested a review May 22, 2023 12:57
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

I am happy with the change now. I will let @joshua-goldstein approve it for merging after review. Thanks for the PR Sarthak.

y/error_test.go Outdated Show resolved Hide resolved
@SarthakMakhija
Copy link
Contributor Author

I am happy with the change now. I will let @joshua-goldstein approve it for merging after review. Thanks for the PR Sarthak.

Welcome Aman :). Thank you for all the help.

@mangalaman93 mangalaman93 self-requested a review May 22, 2023 18:45
@mangalaman93 mangalaman93 merged commit 4b0d919 into dgraph-io:main Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: *DB.Sync should sync memtable in addition to value log
4 participants