Skip to content

fix crash loop on missing manifest tables #2198

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

Merged
merged 2 commits into from
May 14, 2025

Conversation

RJKeevil
Copy link
Contributor

@RJKeevil RJKeevil commented May 11, 2025

Please see https://discuss.hypermode.com/t/manifest-removes-non-existing-table/19882

W are experiencing an error where dgraph tries to remove a table that is missing. I assume that given it has already been removed, it should not trigger a crash loop in compaction. Instead i sense check that the table is indeed removed from all levels, and return without an error code. Please advise if you think this is safe or not.

Checklist

  • Code compiles correctly and linting passes locally

@RJKeevil RJKeevil requested a review from a team as a code owner May 11, 2025 19:04
@RJKeevil
Copy link
Contributor Author

RJKeevil commented May 13, 2025

@mangalaman93 the test error seems related to the use of t.Cleanup, can you advise?

@mangalaman93
Copy link
Member

@mangalaman93 the test error seems related to the use of t.Cleanup, can you advise?

        defer removeDir(dir)
	deletionsThreshold := 10
	mf, m, err := helpOpenOrCreateManifestFile(dir, false, 0, deletionsThreshold)

	db, err := Open(DefaultOptions(dir))
	require.NoError(t, err, "error while opening db")

	t.Cleanup(func() {
		require.NoError(t, db.Close())
	})

it seems that defer cleaned up the directory and db.Close is not happy about it. I think we should use either t.Cleanup everywhere in this test or defer.

@RJKeevil
Copy link
Contributor Author

I went back to defer for now to have control over their order, lets see if the tests now pass

@RJKeevil
Copy link
Contributor Author

and again, with the remove explicitly in the same defer as close db (since they are coupled anyway)

@mangalaman93 mangalaman93 merged commit 674c0a7 into hypermodeinc:main May 14, 2025
3 checks passed
@mangalaman93
Copy link
Member

Thank you for the PR @RJKeevil

@RJKeevil
Copy link
Contributor Author

Thanks for the help @mangalaman93 !

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.

2 participants