Skip to content

Ensure partial bulks released if channel closes#112724

Merged
Tim-Brooks merged 7 commits intoelastic:partial-rest-requestsfrom
Tim-Brooks:incremental_rest_ensure_resources_released
Sep 12, 2024
Merged

Ensure partial bulks released if channel closes#112724
Tim-Brooks merged 7 commits intoelastic:partial-rest-requestsfrom
Tim-Brooks:incremental_rest_ensure_resources_released

Conversation

@Tim-Brooks
Copy link
Copy Markdown
Contributor

@Tim-Brooks Tim-Brooks commented Sep 10, 2024

Currently, the entire close pipeline is not hooked up in case of a
channel close while a request is being buffered or executed. This commit
resolves the issue by adding a connection to a stream closure.

@Tim-Brooks
Ensure partial bulks released if channel closes
46b3ece 
Currently, the entire close pipeline is not hooked up in case of a
channel close while a request is being buffered or executed. This commit
resolves the issue by adding a connection to a stream closure.
@Tim-Brooks
Changes
2ea1c82
@Tim-Brooks Tim-Brooks added >non-issue :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v8.16.0 labels Sep 10, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Sep 10, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Sep 10, 2024

Pinging @elastic/es-distributed (Team:Distributed)

@mhl-b
Copy link
Copy Markdown
Contributor

mhl-b commented Sep 10, 2024 •
edited
Loading

Do you need a separate method for closing? What about sending empty last content or poison pill to handler onNext, sort of EOF. That should terminate handler, probably with exception.

@Tim-Brooks
Copy link
Copy Markdown
Contributor Author

Tim-Brooks commented Sep 11, 2024

If you're talking about:

public void onNext(ReleasableBytesReference chunk, boolean isLast) {

being used by itself to signal abrupt channel close, it can. I just don't get how that is clean. isLast is not enough as it does not distinguish between healthy end of input and abrupt channel close. Certainly, I could pass onNext(ReleasableBytesReference.ABRUPT_CLOSE_INSTANCE, true). But that does not seem particularly clean.

Another option I considered is that request.contentStream() is accessible from the ChunkHandler. So I could add the functionality for request.contentStream#addCloseListener(Listener). And consumers could optionally use that if they prefer. I believe that same infrastructure could be used for (eventual) circuit breakers.

mhl-b
mhl-b approved these changes Sep 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhl-b mhl-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Tim-Brooks Tim-Brooks merged commit cfa2e88 into elastic:partial-rest-requests Sep 12, 2024
Tim-Brooks added a commit that referenced this pull request Sep 17, 2024
@Tim-Brooks
Ensure partial bulks released if channel closes (#112724)
d2d1eeb 
Currently, the entire close pipeline is not hooked up in case of a
channel close while a request is being buffered or executed. This commit
resolves the issue by adding a connection to a stream closure.
Tim-Brooks added a commit that referenced this pull request Sep 18, 2024
@Tim-Brooks
Ensure partial bulks released if channel closes (#112724)
bd24626 
Currently, the entire close pipeline is not hooked up in case of a
channel close while a request is being buffered or executed. This commit
resolves the issue by adding a connection to a stream closure.
Tim-Brooks added a commit that referenced this pull request Sep 18, 2024
@Tim-Brooks
Ensure partial bulks released if channel closes (#112724)
58e3a39 
Currently, the entire close pipeline is not hooked up in case of a
channel close while a request is being buffered or executed. This commit
resolves the issue by adding a connection to a stream closure.
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Sep 19, 2024
@Tim-Brooks
Ensure partial bulks released if channel closes (elastic#112724)
c89aba2 
Currently, the entire close pipeline is not hooked up in case of a
channel close while a request is being buffered or executed. This commit
resolves the issue by adding a connection to a stream closure.
@Tim-Brooks Tim-Brooks mentioned this pull request Sep 19, 2024
Merged
Tim-Brooks added a commit that referenced this pull request Sep 20, 2024
@Tim-Brooks
Backport incremental bulk execution (#113215)
69c4a4f 
This commit back ports all of the work introduced in:
#113044

* #111438 - 5e1f655
* #111865 - 478baf1
* #112179 - 1b77421
* #112227 - cbcbc34
* #112267 - c00768a
* #112154 - a03fb12
* #112479 - 95b42a7
* #112608 - ce2d648
* #112629 - 0d55dc6
* #112767 - 2dbbd7d
* #112724 - 58e3a39
* dce8a0b
* #112974 - 92daeeb
* 529d349
* #113161 - e3424bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhl-b mhl-b mhl-b approved these changes

Assignees

No one assigned

Labels

:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >non-issue Team:Distributed Meta label for distributed team. v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Tim-Brooks @elasticsearchmachine @mhl-b @mark-vieira