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.

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

Do not throw an exception if the process finished quickly but without any error. #46073

Merged
merged 2 commits into from
Aug 29, 2019
Merged

Do not throw an exception if the process finished quickly but without any error. #46073

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
240bf82
Do not throw an exception if the process finished quickly but without…
przemekwitek Aug 28, 2019
5b788e3
Apply review comments
przemekwitek Aug 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ...ava/org/elasticsearch/xpack/ml/dataframe/process/MemoryUsageEstimationProcessManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.xpack.core.ml.dataframe.DataFrameAnalyticsConfig;
Expand Down Expand Up @@ -83,9 +84,14 @@ private MemoryUsageEstimationResult runJob(String jobId,
onProcessCrash(jobId, processHolder));
processHolder.process = process;
if (process.isProcessAlive() == false) {
przemekwitek marked this conversation as resolved.
Show resolved Hide resolved
String errorMsg =
new ParameterizedMessage("[{}] Error while starting process: {}", jobId, process.readError()).getFormattedMessage();
throw ExceptionsHelper.serverError(errorMsg);
String processError = process.readError();
// Only throw when there was an actual problem with the process reported as error.
// Otherwise the process might just have been quick enough to finish before process.isProcessAlive() was called.
if (Strings.isNullOrEmpty(processError) == false) {
String errorMsg =
new ParameterizedMessage("[{}] Error while starting process: [{}]", jobId, processError).getFormattedMessage();
throw ExceptionsHelper.serverError(errorMsg);
}
}
try {
return readResult(jobId, process);
Expand Down
28 changes: 23 additions & 5 deletions ...rg/elasticsearch/xpack/ml/dataframe/process/MemoryUsageEstimationProcessManagerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,25 @@ public void testRunJob_EmptyDataFrame() {
verifyNoMoreInteractions(process, listener);
}

public void testRunJob_ProcessNotAlive() {
public void testRunJob_ProcessNotAlive_NoError() throws Exception {
when(process.isProcessAlive()).thenReturn(false);
when(process.readError()).thenReturn("");

processManager.runJobAsync(TASK_ID, dataFrameAnalyticsConfig, dataExtractorFactory, listener);
verify(listener).onResponse(resultCaptor.capture());
MemoryUsageEstimationResult result = resultCaptor.getValue();
assertThat(result, equalTo(PROCESS_RESULT));

InOrder inOrder = inOrder(process);
inOrder.verify(process).isProcessAlive();
inOrder.verify(process).readError();
inOrder.verify(process).readAnalyticsResults();
inOrder.verify(process).consumeAndCloseOutputStream();
inOrder.verify(process).close();
verifyNoMoreInteractions(process, listener);
}

public void testRunJob_ProcessNotAlive_Error() {
when(process.isProcessAlive()).thenReturn(false);
when(process.readError()).thenReturn("Error from inside the process");

Expand All @@ -103,11 +121,11 @@ public void testRunJob_ProcessNotAlive() {
ElasticsearchException exception = (ElasticsearchException) exceptionCaptor.getValue();
assertThat(exception.status(), equalTo(RestStatus.INTERNAL_SERVER_ERROR));
assertThat(exception.getMessage(), containsString(TASK_ID));
assertThat(exception.getMessage(), containsString("Error while starting process"));
assertThat(exception.getMessage(), containsString("Error from inside the process"));
assertThat(exception.getMessage(), containsString("Error while starting process: [Error from inside the process]"));

verify(process).isProcessAlive();
verify(process).readError();
InOrder inOrder = inOrder(process);
inOrder.verify(process).isProcessAlive();
inOrder.verify(process).readError();
verifyNoMoreInteractions(process, listener);
}

Expand Down