-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
[DATA-FRAME-ANALYTICS] Add task recovery on node change #39416
[DATA-FRAME-ANALYTICS] Add task recovery on node change #39416
Conversation
Pinging @elastic/ml-core |
@@ -100,6 +101,7 @@ task internalClusterTest(type: RandomizedTestingTask, | |||
dependsOn: unitTest.dependsOn) { | |||
include '**/*IT.class' | |||
systemProperty 'es.set.netty.runtime.available.processors', 'false' | |||
systemProperties 'java.security.policy': file("src/main/plugin-metadata/plugin-security-test.policy").absolutePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to add these two lines for the integration tests to run. Apparently, with how the internal cluster is built, the painless plugin is not included, so I included it, and add the requisite java security policy so that it can run.
If there is a better way (or if the tests are deemed unnecessary), then I will happily change this.
.../src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDataFrameAnalyticsAction.java
Show resolved
Hide resolved
* | ||
* @return Set of Strings representing the result fields for the constructed analysis | ||
*/ | ||
Set<String> getResultFields(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think a better solution here is to have some known prefix for our result fields and which analyzer they satisfy.
Something like _ml_analysis_result.<analyzer>.<result-name>
. But, that change is more substantial, opted for better planning and refactoring down the line when/if we make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a few minor comments.
.../src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDataFrameAnalyticsAction.java
Show resolved
Hide resolved
.../main/java/org/elasticsearch/xpack/ml/dataframe/extractor/DataFrameDataExtractorFactory.java
Outdated
Show resolved
Hide resolved
...gin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/DataFrameAnalyticsManagerIT.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This change is fairly simple (for now) as we don't have any state to really recover. We simply need to check what the previous state was and take some simple actions ahead of time.