Conversation
…ifests manifest_evaluator returns one _ManifestEvalVisitor, and callers such as _OverwriteFiles._deleted_entries evaluate the parent snapshot's manifests on the ExecutorFactory thread pool. eval stored the manifest's partition summaries on the instance before visiting the filter, so a thread switch between storing and reading them made one manifest be judged by another manifest's bounds. When the mis-judged manifest held the files being deleted it was skipped, those files were never found, and the commit failed with "Missing required files to delete" although the files were live. Evaluating on a shallow copy keeps the bound filter shared and read-only while giving each call its own summaries. Closes apache#4031
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4031.
The bug
manifest_evaluatorbuilds a single_ManifestEvalVisitorper partition spec and returns its boundeval._OverwriteFiles._deleted_entriesthen calls that one object from theExecutorFactorythread pool, whileevalkept the per-manifest state on the instance:If one thread stores manifest A's summaries and another stores manifest B's before A reads its first predicate leaf, A is judged by B's partition bounds. When A holds the files being deleted it is skipped,
_validate_required_deletesnever finds them, and the overwrite fails withValidationException: Missing required files to deleteeven though the files are live. The reporter measures roughly 1 in 700 overwrites on production tables with many manifests, each one discarding the commit and its work.The opposite direction — keeping a manifest that should have been skipped — is harmless, because the entry comparison finds nothing in it.
The fix
Evaluate on a shallow copy, so the bound filter stays shared and read-only while each call gets its own summaries. This is the first of the three options @QlikFrederic suggested, and it leaves the hot path allocating one small object per manifest rather than rebinding the filter.
As far as I can see
_deleted_entriesis the only caller that shares an evaluator across threads —DataScan.plan_files,_existing_manifestsand_DeleteFilesevaluate one manifest at a time — but the fix belongs inevaleither way, since the object is reachable from the factory and nothing about its contract says single-threaded.The test
test_manifest_evaluator_judges_each_manifest_by_its_own_summariesforces the interleaving deterministically rather than racing: it holds the thread evaluating the in-range manifest at its first predicate leaf, after the summaries are stored, until a second thread has evaluated an out-of-range manifest. No sleeps, and it uses theThreadPoolExecutor/Eventidiom already present in this test module.It fails on
mainwithassert Falseon "manifestholds id == INT_MIN_VALUE and must be read" and passes with the fix.Verification
Run locally against
tests/expressions/test_visitors.py:main: 72 passedI could not run the full suite locally because
tests/conftest.pyimportsmoto, which I could not install in this environment, so those runs used--noconftest; that accounts for 19 identical fixture-lookup errors in both the before and after runs, and it is why the counts differ by exactly the one added test. CI will cover the rest.