This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
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.
Await un-partial-stating after a partial-state join #12399
Await un-partial-stating after a partial-state join #12399
Changes from 6 commits
87e4907
5e0ebc0
df5a3c4
13c8691
d496e95
3765f7a
8739873
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.
Not particularly fussy, but 'un-partial-stated' made me grin; I would have called this completed? 😀
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.
maybe? it would have to be
notify_event_state_completed
or something, to distinguish from other types of completion. Having a single concept which we refer to throughout the codebase as "partial state" seems clearer to me.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.
You might have a point there, even if 'un-partial' is a little non-obvious, once you notice what it means it makes sense.
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.
the intention is to follow up with further PRs which relax this by setting
await_full_state=False
where it is known to be safe to do so.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.
I appreciate the need for this, but this seems like it could block for a long time, potentially exceeding the timeout of a request.
I suppose the plan is to make enough endpoints not need to wait for full state that this won't matter much?
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.
That's part of the plan. Another part of the plan is to get cancellation working (#3528) so that we don't stack up requests. A final part of the plan is probably to change the api to do something different, but we're a way off that yet.