Skip to content

[ML] Better interrupt handling during named pipe connection #1311

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 17, 2020
Merged

[ML] Better interrupt handling during named pipe connection #1311

merged 5 commits into from
Jun 17, 2020

Conversation

@droberts195
Copy link

@droberts195 droberts195 commented Jun 11, 2020

This change fixes two related issues with named pipe connection
when the controller process first starts up:

  1. If the ES JVM dies before the controller connects its logging
    named pipe then since Prevent unexpected native controller output hanging the process elasticsearch#56491 the resulting
    errors from the controller process will be seen in the ES
    stderr file. There is a change to the logging to make it
    clearer that the controller didn't fail, but exited due to
    the ES JVM disappearing.
  2. Interrupted system calls while connecting the named pipes
    could cause the pipes to unnecessarily fail to connect. There
    is a change to retry the calls on getting an EINTR error unless
    the interrupt was caused by the functionality of controller
    that kills off the connection attempts if the ES JVM dies (i.e.
    the scenario described in point 1).

Relates #564
Relates elastic/elasticsearch#57969
[ML] Better interrupt handling during named pipe connection
bde6d29 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Relates elastic#564 Relates elastic/elasticsearch#57969
@droberts195 droberts195 requested a review from davidkyle June 16, 2020 13:55
davidkyle
davidkyle approved these changes Jun 17, 2020
View reviewed changes
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

left a question
bin/controller/CBlockingCallCancellerThread.h Outdated
CBlockingCallCancellerThread(core::CThread::TThreadId potentiallyBlockedThreadId,
std::istream& monitorStream);

const volatile std::atomic_bool& hasCancelledBlockingCall() const;
Copy link
Member

@davidkyle davidkyle Jun 17, 2020

Choose a reason for hiding this comment

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

A volatile atomic is quite pessimistic isn't it

This one is difficult to google, why aren't the memory guarantees of the atomic sufficient here?
Copy link
Author

@droberts195 droberts195 Jun 17, 2020

Choose a reason for hiding this comment

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

Yes, true, volatile is not needed. I will remove it.
@droberts195 droberts195 merged commit af21f3d into elastic:master Jun 17, 2020
@droberts195 droberts195 deleted the better_eintr_handling_in_named_pipe_connection branch June 17, 2020 12:18
droberts195 pushed a commit to droberts195/ml-cpp that referenced this pull request Jun 17, 2020
[7.x][ML] Better interrupt handling during named pipe connection
a8d0254 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Backport of elastic#1311
@droberts195 droberts195 mentioned this pull request Jun 17, 2020
Merged
droberts195 pushed a commit to droberts195/ml-cpp that referenced this pull request Jun 17, 2020
[7.8][ML] Better interrupt handling during named pipe connection
f09c74f 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Backport of elastic#1311
droberts195 pushed a commit to droberts195/ml-cpp that referenced this pull request Jun 17, 2020
[6.8][ML] Better interrupt handling during named pipe connection
f6347e9 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Backport of elastic#1311
This was referenced Jun 17, 2020
Merged
Merged
droberts195 pushed a commit that referenced this pull request Jun 17, 2020
[7.x][ML] Better interrupt handling during named pipe connection (#1324)
5b73507 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Backport of #1311
droberts195 pushed a commit that referenced this pull request Jun 18, 2020
[6.8][ML] Better interrupt handling during named pipe connection (#1326)
92fdad5 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Backport of #1311
droberts195 pushed a commit that referenced this pull request Jun 18, 2020
[7.8][ML] Better interrupt handling during named pipe connection (#1325)
a732216 
This change fixes two related issues with named pipe connection when the controller process first starts up: 1. If the ES JVM dies before the controller connects its logging named pipe then since elastic/elasticsearch#56491 the resulting errors from the controller process will be seen in the ES stderr file. There is a change to the logging to make it clearer that the controller didn't fail, but exited due to the ES JVM disappearing. 2. Interrupted system calls while connecting the named pipes could cause the pipes to unnecessarily fail to connect. There is a change to retry the calls on getting an EINTR error unless the interrupt was caused by the functionality of controller that kills off the connection attempts if the ES JVM dies (i.e. the scenario described in point 1). Backport of #1311
@tveasey tveasey mentioned this pull request Dec 8, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :ml v6.8.11 v7.8.1 v7.9.0 v8.0.0

2 participants

@droberts195 @davidkyle