Skip to content

Conversation

droberts195
Copy link
Contributor

This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Before this change if the ES JVM didn't connect to one of
the named pipes of an autodetect, normalize or
data_frame_analyzer process then it would hang forever.

After this change if nothing connects to the other end of
a named pipe that the C++ process has created within the
timeout period then it will delete the named pipe and exit.

The timeout will be supplied on the command line by a
followup Java PR. In the meantime, (unreleased snapshot)
builds that have the C++ change but not the Java change will
use a timeout of 5 minutes, which is better than nothing and
highly unlikely to be less than the configured process
connect timeout in the ES settings.

Relates #1504

This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Before this change if the ES JVM didn't connect to one of
the named pipes of an autodetect, normalize or
data_frame_analyzer process then it would hang forever.

After this change if nothing connects to the other end of
a named pipe that the C++ process has created within the
timeout period then it will delete the named pipe and exit.

The timeout will be supplied on the command line by a
followup Java PR.  In the meantime, (unreleased snapshot)
builds that have the C++ change but not the Java change will
use a timeout of 5 minutes, which is better than nothing and
highly unlikely to be less than the configured process
connect timeout in the ES settings.

Relates elastic#1504
class CBlockingCallCancellingTimer : public CBlockingCallCancellerThread {
public:
//! Default timeout if not specified.
static const ml::core_t::TTime DEFAULT_TIMEOUT_SECONDS;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to stick with TTime here rather than switch to std::chrono::duration. The reason is that all the existing command line argument parsing is currently using TTime and taking whole numbers of seconds, and it would be inconsistent if the new command line argument tried to do something clever with generic durations.

I think at some point we do need to move over to the std::chrono stuff throughout the codebase. This is obviously a huge project but unless we do it we're going to have messy boundary points like this. It would also allow us to support sub-second bucket spans more naturally than the scaling approach previously envisaged. However, the time to do it is probably once we've upgraded to C++20 when the standard library will have support for parsing durations from text (as well as support for parsing timestamps - see #1459).

droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Sep 29, 2020
This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
This argument requires the changes of elastic/ml-cpp#1514 in
order to work, so that PR will be merged before this one.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Closes elastic/ml-cpp#1504
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 droberts195 merged commit 7930716 into elastic:master Sep 29, 2020
@droberts195 droberts195 deleted the timeout_for_named_pipe_connections branch September 29, 2020 11:28
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Sep 29, 2020
This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Before this change if the ES JVM didn't connect to one of
the named pipes of an autodetect, normalize or
data_frame_analyzer process then it would hang forever.

After this change if nothing connects to the other end of
a named pipe that the C++ process has created within the
timeout period then it will delete the named pipe and exit.

The timeout will be supplied on the command line by a
followup Java PR.  In the meantime, (unreleased snapshot)
builds that have the C++ change but not the Java change will
use a timeout of 5 minutes, which is better than nothing and
highly unlikely to be less than the configured process
connect timeout in the ES settings.

Backport of elastic#1514
droberts195 added a commit that referenced this pull request Sep 29, 2020
This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Before this change if the ES JVM didn't connect to one of
the named pipes of an autodetect, normalize or
data_frame_analyzer process then it would hang forever.

After this change if nothing connects to the other end of
a named pipe that the C++ process has created within the
timeout period then it will delete the named pipe and exit.

The timeout will be supplied on the command line by a
followup Java PR.  In the meantime, (unreleased snapshot)
builds that have the C++ change but not the Java change will
use a timeout of 5 minutes, which is better than nothing and
highly unlikely to be less than the configured process
connect timeout in the ES settings.

Backport of #1514
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Sep 29, 2020
This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
This argument requires the changes of elastic/ml-cpp#1514 in
order to work, so that PR will be merged before this one.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Closes elastic/ml-cpp#1504
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Sep 29, 2020
This PR adds timeouts to the named pipe connections of the
autodetect, normalize and data_frame_analyzer processes.
This argument requires the changes of elastic/ml-cpp#1514 in
order to work, so that PR will be merged before this one.
(The controller process already had a different mechanism,
tied to the ES JVM lifetime.)

Backport of #62993
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