Skip to content

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 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
Contributor

@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 dgraph-io:main May 14, 2025
3 checks passed
@mangalaman93
Copy link
Contributor

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