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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure only a single action for a document is added to a batch #18469
Ensure only a single action for a document is added to a batch #18469
Changes from all commits
f8bf25b
48db0c8
faed37e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine you've got a sequence of pending actions like
<(Upload A), (Upload B), (Merge A), (Upload C), (Merge B)>
.The code as implemented will create a batch of
<(Upload A), (Upload B)>
and leave<(Merge A), (Upload C), (Merge B)>
pending for the next batch.I was hoping we could instead create a batch of
<(Upload A), (Upload B), (Upload C)>
and leave<(Merge A), (Merge B)>
pending for the next batch.It would be a little more complicated because .NET's Queue isn't amenable to cutting in line. It won't be a breaking change to do this in the future though, so let's ship what you have here and consider fixing this as an optimization next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I was under the (wrong) impression that the order cannot be mucked with. But it makes sense that only the order of operations for the same document need be respected.
Can there ever be a case that relies on the ordering of the whole set of operations? Like, (a completely made up example) - there's a hook on the service side that executes additional operations and relies on the relationship between the documents? So,
(Upload C)
becomes an issue if it happens before(Merge A)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little tricky. Would it be easier to have a
List<string> pendingKeys
collection that you add/remove from in these methods and thenCollectionAssert.AllItemsAreUnique
in each?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Let me see how I can make the checks simpler.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.