Filter out fully acknowledged split assignments #14668
Merged
Conversation
This file contains 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
| @@ -589,7 +589,6 @@ public void enqueueSplits(Set<ScheduledSplit> splits, boolean noMoreSplits) | |||
| verify(driverFactory.getSourceId().isPresent(), "not a source driver"); | |||
| verify(!this.noMoreSplits.get() || splits.isEmpty(), "cannot add splits after noMoreSplits is set"); | |||
| queuedSplits.addAll(splits); | |||
| verify(!this.noMoreSplits.get() || noMoreSplits, "cannot unset noMoreSplits"); | |||
There was a problem hiding this comment.
Would you like to add a document here to explain the problem?
There was a problem hiding this comment.
Yeah - please add a sentence of explanation that here we can get non empty splits list even if we already transitioned to noMoreSplits state, and this is not a big deal as those will be filtered out later.
Actually we can probably drop it here already.
There was a problem hiding this comment.
Oh wait - those are filtered out before, not after the call :) Please just note that in the comment.
losipiuk
approved these changes
Oct 18, 2022
- Rename local variable - Use toImmutableSet collector to avoid creating a copy in the SplitAssignment constructor
Here is a sequence of interaction that leads to a failure at the `!this.noMoreSplits.get() || noMoreSplits` check in DriverSplitRunnerFactory#enqueueSplits: - The coordinator sends a TaskUpdateRequest[1] containing some splits and a noMoreSplits set to false - A TaskUpdateRequest[1] times out on coordinator and coordinator re-sends it as TaskUpdateRequest[1'] - The TaskUpdateRequest[1'] succeeds and the coordinator sends a TaskUpdateRequest[2] with noMoreSplits set to true. - The TaskUpdateRequest[2] succeeds - The original TaskUpdateRequest[1] arrives to the worker node. The splits in this requests are getting filtered out by the logic in the updateSplitAssignments - DriverSplitRunnerFactory#enqueueSplits is called with an empty list of splits, so the first (really important check) "!this.noMoreSplits.get() || splits.isEmpty()" passes, but the second check (not so important) "!this.noMoreSplits.get() || noMoreSplits" fails what leads to a task failure To avoid a failure of the check mentioned we filter out fully acknowledged SplitAssignment's (ones that contain no unacknowledged splits)
arhimondr
force-pushed
the
remove-unnecessary-no-more-splits-check
branch
from
71b9fc2 to
8dbddb2
Compare
arhimondr
changed the title
|
Instead of writing an elaborative comment I decided to simply fix the issue. Now I'm filtering out split assignments that are fully acknowledged. @linzebing @losipiuk Please let me know what you think. |
linzebing
approved these changes
Oct 21, 2022
zhengxingmao
approved these changes
Oct 21, 2022
losipiuk
approved these changes
Oct 21, 2022
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.
Here is a sequence of interaction that leads to a failure at the
!this.noMoreSplits.get() || noMoreSplitscheck inDriverSplitRunnerFactory#enqueueSplits:
set to false
re-sends it as TaskUpdateRequest[1']
with noMoreSplits set to true.
splits in this requests are getting filtered out by the logic in the updateSplitAssignments
splits, so the first (really important check) "!this.noMoreSplits.get() || splits.isEmpty()"
passes, but the second check (not so important) "!this.noMoreSplits.get() || noMoreSplits" fails
what leads to a task failure
To avoid a failure of the check mentioned we filter out fully
acknowledged SplitAssignment's (ones that contain no unacknowledged
splits)
Description
Non-technical explanation
com.google.common.base.VerifyException: cannot unset noMoreSplitsRelease notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: