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] Create the ml-config index #36608

Closed

Conversation

davidkyle
Copy link
Member

@davidkyle davidkyle commented Dec 13, 2018

Create the .ml-config index in the migrator class if it does not exist and require the existence of the index for the jobs/datafeeds are eligible to be migrated checks. The index is created only if there are job and datafeed configs to migrate.

The created index has index.max_result_window explicitly set to 10,000 and the template has the same setting. The classes that search the index for configs use that value to set the search size.

Addressed #34864

@davidkyle davidkyle added WIP :ml Machine learning labels Dec 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@davidkyle davidkyle force-pushed the jindex-create-index branch 3 times, most recently from 13d67ca to 5a74edf Compare December 17, 2018 12:37
@davidkyle davidkyle mentioned this pull request Dec 17, 2018
43 tasks
@davidkyle davidkyle force-pushed the jindex-create-index branch 2 times, most recently from e0e73a1 to fabe447 Compare December 17, 2018 18:40
@davidkyle davidkyle removed the WIP label Dec 17, 2018
@davidkyle
Copy link
Member Author

Retest this please

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

Left a few comments.

@@ -90,6 +94,7 @@
private static final Logger logger = LogManager.getLogger(MlConfigMigrator.class);

public static final String MIGRATED_FROM_VERSION = "migrated from version";
public static final int CONFIG_INDEX_MAX_RESULTS_WINDOW = 10_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

This setting is misplaced here. Could you move it to AnomalyDetectorsIndex please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should take the chance and raise this to 100K?

* datafeed configuration document
*
* The number of datafeeds returned in a search it limited to
* {@link MlConfigMigrator#CONFIG_INDEX_MAX_RESULTS_WINDOW}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need updating once the setting is moved.

@@ -89,6 +90,11 @@
/**
* This class implements CRUD operation for the
* anomaly detector job configuration document
*
* The number of jobs returned in a search it limited to
* {@link MlConfigMigrator#CONFIG_INDEX_MAX_RESULTS_WINDOW}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need updating once the setting is moved.

@davidkyle
Copy link
Member Author

run gradle build tests 2

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

LGTM

@davidkyle davidkyle changed the base branch from feature-jindex-6x to 6.x December 18, 2018 17:46
@davidkyle davidkyle changed the base branch from 6.x to feature-jindex-6x December 18, 2018 17:52
@davidkyle davidkyle changed the base branch from feature-jindex-6x to 6.x December 18, 2018 17:56
@davidkyle davidkyle changed the base branch from 6.x to feature-jindex-6x December 18, 2018 17:57
@davidkyle
Copy link
Member Author

Closing as #36792 replaces this

@davidkyle davidkyle closed this Dec 18, 2018
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.

3 participants