Skip to content

Conversation

@dimitris-athanasiou
Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou commented Jan 28, 2019

Converts data frame analytics to run as persistent tasks.

Adds the following APIs:

  • PUT _ml/data_frame/analysis/{id}
  • GET _ml/data_frame/analysis/{id}
  • GET _ml/data_frame/analysis/{id}/_stats
  • POST _ml/data_frame/analysis/{id}/_start
  • DELETE _ml/data_frame/analysis/{id}

@dimitris-athanasiou dimitris-athanasiou added WIP :ml Machine learning labels Jan 28, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@dimitris-athanasiou dimitris-athanasiou force-pushed the dataframe-analytics-persistent-tasks branch from 0d538ed to 22afa85 Compare January 31, 2019 18:15
@dimitris-athanasiou
Copy link
Contributor Author

I have now updated this PR with some tests. There will definitely be more coming but I believe at this point there's enough to get this merged.

@dimitris-athanasiou
Copy link
Contributor Author

run elasticsearch-ci/2

Copy link
Member

Choose a reason for hiding this comment

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

👍 putting the validations outside of the model

Choose a reason for hiding this comment

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

This isn't needed to delete the configs because the base class will delete the index they're stored in, so they'll all get wiped between tests.

The only thing this is doing is possibly triggering the error that delete throws when you try to delete an analytics process that's running. If that's the reason this method exists then I think it should be renamed to reflect that.

But maybe ESRestTestCase.waitForPendingTasks() already does that check? In which case there is no need to have this method at all.

(As an aside, the anomaly detector and datafeed methods in this class will no longer need to call delete once we're sure every config is in an index, and then those methods can be renamed. But that's for a different PR.)

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

👍 Some discussion topics/future concerns but looks good.

Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure that in the future we remember to add these new tasks to our upgrade_mode settings. So that when users upgrade from 7.x -> 8.x they can do so without stopping these tasks themselves.

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'll add a TODO.

Copy link
Member

Choose a reason for hiding this comment

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

When we have a FAILED state, adding a REASON field would be very helpful for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no proper handling of failures yet. I'll make sure to add the reason in when I add failure handling.

Copy link

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

It might be practical to break out the generic parts and PR them to master, so we can use them before the feature branch is merged

@dimitris-athanasiou
Copy link
Contributor Author

@hendrikmuhs I think we need a usage in master before porting the abstract-get-action. I'll try to use it for get-filters soon.

Copy link

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

Since this is only going into a feature branch I'm happy to merge it now and resolve everything that's left in followups

@dimitris-athanasiou dimitris-athanasiou force-pushed the dataframe-analytics-persistent-tasks branch from 0d035a8 to 2ac526d Compare February 4, 2019 09:37
@dimitris-athanasiou dimitris-athanasiou merged commit 551f9a8 into elastic:feature-ml-data-frame-analytics Feb 4, 2019
@dimitris-athanasiou dimitris-athanasiou deleted the dataframe-analytics-persistent-tasks branch February 4, 2019 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:ml Machine learning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants