Skip to content
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

Close task after calling sendResponse() #94865

Conversation

DaveCTurner
Copy link
Contributor

Today we unregister transport tasks before calling TransportChannel#sendResponse in order to send back the response. Yet the serialization and transmission work is still really part of the task's work, so we should keep the task around until these parts are done too.

This commit delays the task unregistration until after sendResponse returns, which is simple to achieve. Delaying it until the end of transmission is a little harder and will be the subject of future work.

Relates #94845

Today we unregister transport tasks before calling
`TransportChannel#sendResponse` in order to send back the response. Yet
the serialization and transmission work is still really part of the
task's work, so we should keep the task around until these parts are
done too.

This commit delays the task unregistration until after `sendResponse`
returns, which is simple to achieve. Delaying it until the end of
transmission is a little harder and will be the subject of future work.

Relates elastic#94845
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Network Http and internode communication implementations v8.8.0 labels Mar 29, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Mar 29, 2023
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM fine by me if CI is happy :)

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 29, 2023
@elasticsearchmachine elasticsearchmachine merged commit 24b28d0 into elastic:main Mar 29, 2023
@DaveCTurner DaveCTurner deleted the 2023-03-29-close-task-after-channel-sendResponse branch March 29, 2023 13:10
elasticsearchmachine pushed a commit that referenced this pull request Apr 11, 2023
Since #94865 task
unregistration  is not guaranteed to have happened upon receiving the
response, e.g. for a  `internal:transport/handshake` when connecting the
test nodes. Therefore,  we need to wait for ongoing tasks to finish.

closes #94987
arteam added a commit to arteam/elasticsearch that referenced this pull request Apr 26, 2023
Due to elastic#94865, we now send an ack before the task gets removed, so we can see
a non-zero amount of tasks in the task manager for a short amount of time.

We can just busy wait until the task gets removed.

See elastic#95494, elastic#94987
arteam added a commit that referenced this pull request May 9, 2023
* Fix TasksIT.testTasksUnblocking

Due to #94865, we now send an ack before the task gets removed, so we can see
a non-zero amount of tasks in the task manager for a short amount of time.

We can just busy wait until the task gets removed.

See #95494, #94987
elasticsearchmachine pushed a commit that referenced this pull request May 25, 2023
I think this also relates to reversing the order of sending response and
unregistration in #94865.

Closes #96103
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Network Http and internode communication implementations >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants