Skip to content

[ML] Make controller send responses for each command received #1520

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

Merged
merged 7 commits into from
Oct 17, 2020

Conversation

droberts195
Copy link
Contributor

@droberts195 droberts195 commented Oct 1, 2020

This change makes the controller process respond to each command
it receives with a document indicating whether that command was
successfully executed or not.

This response will be used by the Java side of the connection to
determine when it is appropriate to move on to the next phase of
the action that the controller command was part of. For example,
when starting a process and connecting named pipes to it it is
best that the named pipe connections are not attempted until the
process is confirmed to be started.

Relates elastic/elasticsearch#62823
Relates elastic/elasticsearch#63542

This change makes the controller process respond to each command
it receives with a document indicating whether that command was
successfully executed or not.

This response will be used by the Java side of the connection to
determine when it is appropriate to move on to the next phase of
the action that the controller command was part of.  For example,
when starting a process and connecting named pipes to it it is
best that the named pipe connections are not attempted until the
process is confirmed to be started.

Relates elastic/elasticsearch#62823
By writing results in an array it's possible to reuse the Java
ProcessResultsParser class to read them.

Also, making the output thread safe will make implementing elastic#1503
easier if that's ever done.
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Oct 12, 2020
This change makes threads that send a command to the ML
controller process wait for it to respond to the command.
Previously such threads would block until the command was
sent, but not until it was actioned.  This was on the
assumption that the sort of commands being sent would be
actioned almost instantaneously, but that assumption has
been shown to be false when anti-malware software is
running.

Relates elastic/ml-cpp#1520
Fixes elastic#62823
@droberts195 droberts195 marked this pull request as ready for review October 12, 2020 09:49
@droberts195 droberts195 requested a review from edsavage October 13, 2020 09:22
Copy link
Contributor

@edsavage edsavage left a comment

Choose a reason for hiding this comment

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

LGTM

droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Oct 13, 2020
This is to allow merging of elastic/ml-cpp#1520 and elastic#63542
without causing every single CI build to fail.

These changes will be reverted in elastic#63542 after downloadable
snapshots containing the changes of elastic/ml-cpp#1520 are
available.
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Oct 13, 2020
This change makes the controller process respond to each command
it receives with a document indicating whether that command was
successfully executed or not.

This response will be used by the Java side of the connection to
determine when it is appropriate to move on to the next phase of
the action that the controller command was part of. For example,
when starting a process and connecting named pipes to it it is
best that the named pipe connections are not attempted until the
process is confirmed to be started.

Backport of elastic#1520
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 16, 2020
This is to allow merging of elastic/ml-cpp#1520 and #63542
without causing every single CI build to fail.

These changes will be reverted in #63542 after downloadable
snapshots containing the changes of elastic/ml-cpp#1520 are
available.
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 16, 2020
This is to allow merging of elastic/ml-cpp#1520 and #63542
without causing every single CI build to fail.

These changes will be reverted in #63542 after downloadable
snapshots containing the changes of elastic/ml-cpp#1520 are
available.

Backport of #63610
@droberts195
Copy link
Contributor Author

retest

@droberts195 droberts195 merged commit d90ff2e into elastic:master Oct 17, 2020
@droberts195 droberts195 deleted the controller_responses branch October 17, 2020 06:30
droberts195 added a commit that referenced this pull request Oct 17, 2020
…1534)

This change makes the controller process respond to each command
it receives with a document indicating whether that command was
successfully executed or not.

This response will be used by the Java side of the connection to
determine when it is appropriate to move on to the next phase of
the action that the controller command was part of. For example,
when starting a process and connecting named pipes to it it is
best that the named pipe connections are not attempted until the
process is confirmed to be started.

Backport of #1520
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 17, 2020
This change makes threads that send a command to the ML
controller process wait for it to respond to the command.
Previously such threads would block until the command was
sent, but not until it was actioned.  This was on the
assumption that the sort of commands being sent would be
actioned almost instantaneously, but that assumption has
been shown to be false when anti-malware software is
running.

Relates elastic/ml-cpp#1520
Fixes #62823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants