Skip to content

Commit 8cc4194

Browse files
przemekwitekprzemekwitek
authored
Do not throw an exception if the process finished quickly but without any error. (#46073)
1 parent 02abb1a commit 8cc4194

File tree

2 files changed

+49
-50
lines changed

2 files changed

+49
-50
lines changed

‎x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/dataframe/process/MemoryUsageEstimationProcessManager.java

Lines changed: 9 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,10 @@
1717
import org.elasticsearch.xpack.ml.dataframe.extractor.DataFrameDataExtractorFactory;
1818
import org.elasticsearch.xpack.ml.dataframe.process.results.MemoryUsageEstimationResult;
1919

20-
import java.io.IOException;
2120
import java.util.Iterator;
2221
import java.util.Objects;
2322
import java.util.Set;
2423
import java.util.concurrent.ExecutorService;
25-
import java.util.function.Consumer;
2624

2725
public class MemoryUsageEstimationProcessManager {
2826

@@ -74,24 +72,21 @@ private MemoryUsageEstimationResult runJob(String jobId,
7472
"",
7573
categoricalFields,
7674
config.getAnalysis());
77-
ProcessHolder processHolder = new ProcessHolder();
7875
AnalyticsProcess<MemoryUsageEstimationResult> process =
7976
processFactory.createAnalyticsProcess(
8077
jobId,
8178
processConfig,
8279
executorServiceForProcess,
83-
onProcessCrash(jobId, processHolder));
84-
processHolder.process = process;
85-
if (process.isProcessAlive() == false) {
86-
String errorMsg =
87-
new ParameterizedMessage("[{}] Error while starting process: {}", jobId, process.readError()).getFormattedMessage();
88-
throw ExceptionsHelper.serverError(errorMsg);
89-
}
80+
// The handler passed here will never be called as AbstractNativeProcess.detectCrash method returns early when
81+
// (processInStream == null) which is the case for MemoryUsageEstimationProcess.
82+
reason -> {});
9083
try {
9184
return readResult(jobId, process);
9285
} catch (Exception e) {
9386
String errorMsg =
94-
new ParameterizedMessage("[{}] Error while processing result [{}]", jobId, e.getMessage()).getFormattedMessage();
87+
new ParameterizedMessage(
88+
"[{}] Error while processing process output [{}], process errors: [{}]",
89+
jobId, e.getMessage(), process.readError()).getFormattedMessage();
9590
throw ExceptionsHelper.serverError(errorMsg, e);
9691
} finally {
9792
process.consumeAndCloseOutputStream();
@@ -101,31 +96,14 @@ private MemoryUsageEstimationResult runJob(String jobId,
10196
LOGGER.info("[{}] Closed process", jobId);
10297
} catch (Exception e) {
10398
String errorMsg =
104-
new ParameterizedMessage("[{}] Error while closing process [{}]", jobId, e.getMessage()).getFormattedMessage();
99+
new ParameterizedMessage(
100+
"[{}] Error while closing process [{}], process errors: [{}]",
101+
jobId, e.getMessage(), process.readError()).getFormattedMessage();
105102
throw ExceptionsHelper.serverError(errorMsg, e);
106103
}
107104
}
108105
}
109106

110-
private static class ProcessHolder {
111-
volatile AnalyticsProcess<MemoryUsageEstimationResult> process;
112-
}
113-
114-
private static Consumer<String> onProcessCrash(String jobId, ProcessHolder processHolder) {
115-
return reason -> {
116-
AnalyticsProcess<MemoryUsageEstimationResult> process = processHolder.process;
117-
if (process == null) {
118-
LOGGER.error(new ParameterizedMessage("[{}] Process does not exist", jobId));
119-
return;
120-
}
121-
try {
122-
process.kill();
123-
} catch (IOException e) {
124-
LOGGER.error(new ParameterizedMessage("[{}] Failed to kill process", jobId), e);
125-
}
126-
};
127-
}
128-
129107
/**
130108
* Extracts {@link MemoryUsageEstimationResult} from process' output.
131109
*/

‎x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/dataframe/process/MemoryUsageEstimationProcessManagerTests.java

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package org.elasticsearch.xpack.ml.dataframe.process;
77

88
import org.elasticsearch.ElasticsearchException;
9+
import org.elasticsearch.ElasticsearchParseException;
910
import org.elasticsearch.action.ActionListener;
1011
import org.elasticsearch.common.unit.ByteSizeValue;
1112
import org.elasticsearch.common.util.concurrent.EsExecutors;
@@ -65,7 +66,6 @@ public void setUpMocks() {
6566
executorServiceForJob = EsExecutors.newDirectExecutorService();
6667
executorServiceForProcess = mock(ExecutorService.class);
6768
process = mock(AnalyticsProcess.class);
68-
when(process.isProcessAlive()).thenReturn(true);
6969
when(process.readAnalyticsResults()).thenReturn(List.of(PROCESS_RESULT).iterator());
7070
processFactory = mock(AnalyticsProcessFactory.class);
7171
when(processFactory.createAnalyticsProcess(anyString(), any(), any(), any())).thenReturn(process);
@@ -93,61 +93,61 @@ public void testRunJob_EmptyDataFrame() {
9393
verifyNoMoreInteractions(process, listener);
9494
}
9595

96-
public void testRunJob_ProcessNotAlive() {
97-
when(process.isProcessAlive()).thenReturn(false);
98-
when(process.readError()).thenReturn("Error from inside the process");
96+
public void testRunJob_NoResults() throws Exception {
97+
when(process.readAnalyticsResults()).thenReturn(List.<MemoryUsageEstimationResult>of().iterator());
9998

10099
processManager.runJobAsync(TASK_ID, dataFrameAnalyticsConfig, dataExtractorFactory, listener);
101100

102101
verify(listener).onFailure(exceptionCaptor.capture());
103102
ElasticsearchException exception = (ElasticsearchException) exceptionCaptor.getValue();
104103
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
105104
assertThat(exception.getMessage(), containsString(TASK_ID));
106-
assertThat(exception.getMessage(), containsString("Error while starting process"));
107-
assertThat(exception.getMessage(), containsString("Error from inside the process"));
105+
assertThat(exception.getMessage(), containsString("no results"));
108106

109-
verify(process).isProcessAlive();
110-
verify(process).readError();
107+
InOrder inOrder = inOrder(process);
108+
inOrder.verify(process).readAnalyticsResults();
109+
inOrder.verify(process).readError();
110+
inOrder.verify(process).consumeAndCloseOutputStream();
111+
inOrder.verify(process).close();
111112
verifyNoMoreInteractions(process, listener);
112113
}
113114

114-
public void testRunJob_NoResults() throws Exception {
115-
when(process.readAnalyticsResults()).thenReturn(List.<MemoryUsageEstimationResult>of().iterator());
115+
public void testRunJob_MultipleResults() throws Exception {
116+
when(process.readAnalyticsResults()).thenReturn(List.of(PROCESS_RESULT, PROCESS_RESULT).iterator());
116117

117118
processManager.runJobAsync(TASK_ID, dataFrameAnalyticsConfig, dataExtractorFactory, listener);
118119

119120
verify(listener).onFailure(exceptionCaptor.capture());
120121
ElasticsearchException exception = (ElasticsearchException) exceptionCaptor.getValue();
121122
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
122123
assertThat(exception.getMessage(), containsString(TASK_ID));
123-
assertThat(exception.getMessage(), containsString("no results"));
124+
assertThat(exception.getMessage(), containsString("more than one result"));
124125

125126
InOrder inOrder = inOrder(process);
126-
inOrder.verify(process).isProcessAlive();
127127
inOrder.verify(process).readAnalyticsResults();
128+
inOrder.verify(process).readError();
128129
inOrder.verify(process).consumeAndCloseOutputStream();
129130
inOrder.verify(process).close();
130131
verifyNoMoreInteractions(process, listener);
131132
}
132133

133-
public void testRunJob_MultipleResults() throws Exception {
134-
when(process.readAnalyticsResults()).thenReturn(List.of(PROCESS_RESULT, PROCESS_RESULT).iterator());
134+
public void testRunJob_OneResult_ParseException() throws Exception {
135+
when(process.readAnalyticsResults()).thenThrow(new ElasticsearchParseException("cannot parse result"));
135136

136137
processManager.runJobAsync(TASK_ID, dataFrameAnalyticsConfig, dataExtractorFactory, listener);
137138

138139
verify(listener).onFailure(exceptionCaptor.capture());
139140
ElasticsearchException exception = (ElasticsearchException) exceptionCaptor.getValue();
140141
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
141142
assertThat(exception.getMessage(), containsString(TASK_ID));
142-
assertThat(exception.getMessage(), containsString("more than one result"));
143+
assertThat(exception.getMessage(), containsString("cannot parse result"));
143144

144145
InOrder inOrder = inOrder(process);
145-
inOrder.verify(process).isProcessAlive();
146146
inOrder.verify(process).readAnalyticsResults();
147+
inOrder.verify(process).readError();
147148
inOrder.verify(process).consumeAndCloseOutputStream();
148149
inOrder.verify(process).close();
149150
verifyNoMoreInteractions(process, listener);
150-
151151
}
152152

153153
public void testRunJob_FailsOnClose() throws Exception {
@@ -162,10 +162,32 @@ public void testRunJob_FailsOnClose() throws Exception {
162162
assertThat(exception.getMessage(), containsString("Error while closing process"));
163163

164164
InOrder inOrder = inOrder(process);
165-
inOrder.verify(process).isProcessAlive();
166165
inOrder.verify(process).readAnalyticsResults();
167166
inOrder.verify(process).consumeAndCloseOutputStream();
168167
inOrder.verify(process).close();
168+
inOrder.verify(process).readError();
169+
verifyNoMoreInteractions(process, listener);
170+
}
171+
172+
public void testRunJob_FailsOnClose_ProcessReportsError() throws Exception {
173+
doThrow(ExceptionsHelper.serverError("some LOG(ERROR) lines coming from cpp process")).when(process).close();
174+
when(process.readError()).thenReturn("Error from inside the process");
175+
176+
processManager.runJobAsync(TASK_ID, dataFrameAnalyticsConfig, dataExtractorFactory, listener);
177+
178+
verify(listener).onFailure(exceptionCaptor.capture());
179+
ElasticsearchException exception = (ElasticsearchException) exceptionCaptor.getValue();
180+
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
181+
assertThat(exception.getMessage(), containsString(TASK_ID));
182+
assertThat(exception.getMessage(), containsString("Error while closing process"));
183+
assertThat(exception.getMessage(), containsString("some LOG(ERROR) lines coming from cpp process"));
184+
assertThat(exception.getMessage(), containsString("Error from inside the process"));
185+
186+
InOrder inOrder = inOrder(process);
187+
inOrder.verify(process).readAnalyticsResults();
188+
inOrder.verify(process).consumeAndCloseOutputStream();
189+
inOrder.verify(process).close();
190+
inOrder.verify(process).readError();
169191
verifyNoMoreInteractions(process, listener);
170192
}
171193

@@ -177,7 +199,6 @@ public void testRunJob_Ok() throws Exception {
177199
assertThat(result, equalTo(PROCESS_RESULT));
178200

179201
InOrder inOrder = inOrder(process);
180-
inOrder.verify(process).isProcessAlive();
181202
inOrder.verify(process).readAnalyticsResults();
182203
inOrder.verify(process).consumeAndCloseOutputStream();
183204
inOrder.verify(process).close();

0 commit comments

Comments
 (0)