perf!: improve conflict resolution performance#3882
perf!: improve conflict resolution performance#3882wjones127 merged 9 commits intolance-format:mainfrom
Conversation
wjones127
changed the title
wjones127
force-pushed
the
perf/commit-loop
branch
from
3c055a4 to
a608dd3
Compare
| @Test | ||
| void testCommitConflict() { | ||
| String testMethodName = new Object() {}.getClass().getEnclosingMethod().getName(); | ||
| String datasetPath = tempDir.resolve(testMethodName).toString(); | ||
| try (RootAllocator allocator = new RootAllocator(Long.MAX_VALUE)) { | ||
| TestUtils.SimpleTestDataset testDataset = | ||
| new TestUtils.SimpleTestDataset(allocator, datasetPath); | ||
| try (Dataset dataset = testDataset.createEmptyDataset()) { | ||
| assertEquals(1, dataset.version()); | ||
| assertEquals(1, dataset.latestVersion()); | ||
| assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> { | ||
| testDataset.write(0, 5); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This test didn't make any sense. Append shouldn't generate a commit conflict, almost ever. We might want to add tests back again later with a proper commit conflict.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3882 +/- ##
==========================================
+ Coverage 78.34% 78.41% +0.07%
==========================================
Files 280 280
Lines 107283 107680 +397
Branches 107283 107680 +397
==========================================
+ Hits 84047 84436 +389
- Misses 19912 19917 +5
- Partials 3324 3327 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| use_constant_size_upload_parts: false, | ||
| list_is_lexically_ordered: true, | ||
| io_parallelism: get_num_compute_intensive_cpus(), | ||
| io_parallelism: DEFAULT_CLOUD_IO_PARALLELISM, |
There was a problem hiding this comment.
Why do we want to change this behavior? The original one seems more reasonable for memory store
There was a problem hiding this comment.
This one was surprising to me as well. But because CI runners only have like 4 CPUs, this was limiting the number of parallel requests that could be in flight. It was making tests about num_hops fail. This is really only a problem when you are combining memory store with ThrottledStore.
| .commit_handler | ||
| .list_manifest_locations(&dataset.base, dataset.object_store(), true) | ||
| .try_take_while(move |location| { | ||
| futures::future::ready(Ok(location.version > latest_version)) |
There was a problem hiding this comment.
maybe in future iterations, technically we can also push this down so that the list operation can just list manifests that are after the latest_version and avoid listing all manifests in the version directory.
There was a problem hiding this comment.
The return value of list_manifest_locations is a stream, so it should be efficient when it can be (list operation has lexical order and we are using V2 manifest paths). But often it will be unavoidable to list all manifests.
Though thinking about it, it might be worthwhile doing a fast path at least for local filesystem. The existing implementation finds the latest version (which has a fast local implementation) and then opens all the versions in between. That's probably what we should do for local.
There was a problem hiding this comment.
Would it be possible to leverage the start-after header in S3 when possible so we don't need to list
anything before the latest version?
https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#API_ListObjectsV2_RequestSyntax
There was a problem hiding this comment.
I'm not worried about S3. If you are using V2 manifest paths, then the lexical order is descending (or latest first). So it's just a matter of stopping early to not have to list anything before the current version.
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
looks good to me, thanks for the fix!
#3882 introduced the improvement to list transactions in descending order, and we use the output list to rebase an ongoing transaction. This is logically incorrect because we should rebase in ascending order from oldest transaction to latest transaction. I think currently there is no correctness issue because for merge-insert we do not handle upserts of the same row, and for FRI related operations the handling always sort the related fragment reuse versions internally. But it's still better to fix this logical error which might impact conflict resolution cases we might handle in the future.
3 participants
BREAKING CHANGE: signature of
CommitHandler::list_manifest_locations()has a new parametersorted_descending. It is handled in the default implementation, so no changes are needed unless that method is overridden. Also,ObjectStore::read_dir_all()is now synchronous and returns errors as part of the stream.This PR optimizes the conflict resolution IO pattern.
O(num_transactions)