Parallelize file uploads in fs cp command. #4132
Open
Conversation
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
renaudhartert-db
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: e720670
18 interesting tests: 7 FAIL, 4 RECOVERED, 3 KNOWN, 3 flaky, 1 SKIP
Top 31 slowest tests (at least 2 minutes):
|
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Critical Issue: Context Shadowing Bug
Line 61 in cp.go shadows the context variable from line 57, causing the defer cancel() to cancel the wrong context. This breaks proper cleanup and cancellation propagation.
Issues
-
Context shadowing at line 61: The errgroup.WithContext returns a new context that shadows the cancellable context from line 57. This means defer cancel() on line 58 will cancel the wrong context.
-
Redundant context check at lines 90-92: The ctx.Err() check inside the goroutine is ineffective since the goroutine may start before cancellation, and cpFileToFile already handles context cancellation.
-
Missing test coverage: TestCp_concurrencyValidation only tests invalid values. Should also test valid values (1, 16, 100) work correctly.
-
No integration test for --concurrency flag: The new flag should have an integration test exercising different concurrency values.
Suggestions
-
Consider removing the double context wrapping since errgroup.WithContext already provides cancellation on error.
-
Document the no-ordering-guarantee behavior in command help text.
-
Consider adding debug logs for performance monitoring.
Questions
-
Why the double context wrapping (lines 57 and 61)? Is there a specific reason beyond what errgroup provides?
-
Is concurrency=16 based on benchmarking? Different scenarios might benefit from different values.
-
With concurrent output, messages will be interleaved. Is this acceptable UX?
Review generated by reviewbot
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…bricks/cli into fs-cp-fast
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
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.
What changes are proposed in this pull request?
This PR improves the performance of the
databricks fs cpcommand when copying directories by parallelizing file uploads. The command uses 16 concurrent workers by default but the number can be controlled via--concurrency.Implementation details:
Filerimplementation as before.Why
--concurrency? No strong preference here, it does not seem that there is a pattern in the CLI to control concurrency in other places. This is the flag name used in most Go tools but I'm happy to use something else.How is this tested?
Added acceptance tests to exercise most code paths + unit tests to validate that the context cancellation and propagation works properly.