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.

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

[ML] fix x-pack usage regression caused by index migration #36936

Merged
merged 8 commits into from
Dec 31, 2018

Conversation

hendrikmuhs
Copy link
Contributor

@hendrikmuhs hendrikmuhs commented Dec 21, 2018

Changes the feature usage retrieval to use the job manager rather than
directly talking to the cluster state, because jobs can now be either in
cluster state or stored in an index

This is a follow-up of #36702 / #36698

This is a blocker for 6.6: For every cluster that has at least 1 ML job created with 6.6+ (job configuration persisted in the new config index) the _xpack/usage endpoint is broken which also breaks the collection of usage data.

Notes:

  • i am looking into adding an integration test for this as this has been only found by manual testing

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

Copy link
Contributor

@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.

LGTM

@hendrikmuhs
Copy link
Contributor Author

This PR still fails, because JobManager is not created if the ml plugin is disabled and causes MachineLearningFeatureSet failing to load:

[2018-12-21T16:45:22,991][ERROR][o.e.b.Bootstrap          ] [node-0] Guice Exception: org.elasticsearch.common.inject.CreationException: Guice creation errors:

1) Could not find a suitable constructor in org.elasticsearch.xpack.ml.job.JobManager. Classes must have either one (and only one) constructor annotated with @Inject or a zero-argument constructor that is not private.
  at org.elasticsearch.xpack.ml.job.JobManager.class(Unknown Source)
  while locating org.elasticsearch.xpack.ml.job.JobManager
    for parameter 4 at org.elasticsearch.xpack.ml.MachineLearningFeatureSet.<init>(Unknown Source)
  at _unknown_

1 error
        at <<<guice>>>
        at org.elasticsearch.node.Node.<init>(Node.java:556)
        at org.elasticsearch.node.Node.<init>(Node.java:250)
        at org.elasticsearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:211)
        at org.elasticsearch.bootstrap.Bootstrap.setup(Bootstrap.java:211)
        at org.elasticsearch.bootstrap.Bootstrap.init(Bootstrap.java:325)
        at org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:159)
        at org.elasticsearch.bootstrap.Elasticsearch.execute(Elasticsearch.java:150)
        at org.elasticsearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:86)
        at org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:124)
        at org.elasticsearch.cli.Command.main(Command.java:90)
        at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:115)
        at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:92)

If anyone has a best practice for this case, please let me know.

@droberts195
Copy link
Contributor

I think the solution is to have guice inject a different class, say JobManagerHolder, that contains a reference to a JobManager that is null when ML is disabled. Then MachineLearning.createComponents can return a list containing just a JobManagerHolder (with null JobManager reference) in cases where it currently returns an empty list.

Hendrik Muhs added 8 commits December 28, 2018 09:35
Changed the feature usage retrieval to use the job manager rather than
directly talking to the cluster state, because jobs can now be either in
cluster state or stored in an index
@hendrikmuhs hendrikmuhs merged commit 632c7fb into elastic:master Dec 31, 2018
hendrikmuhs pushed a commit that referenced this pull request Dec 31, 2018
Changes the feature usage retrieval to use the job manager rather than
directly talking to the cluster state, because jobs can now be either in
cluster state or stored in an index

This is a follow-up of #36702 / #36698
hendrikmuhs pushed a commit that referenced this pull request Dec 31, 2018
Changes the feature usage retrieval to use the job manager rather than
directly talking to the cluster state, because jobs can now be either in
cluster state or stored in an index

This is a follow-up of #36702 / #36698
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
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.

5 participants