[ML-DataFrame] version data frame transform internal index #45375

hendrikmuhs · 2019-08-09T11:59:36Z

version the internal data frame transform index to allow smooth upgrades (adding/change mappings)
add mappings for config::version, config::create_time, checkpoint::timestamp, checkpoint::time_upper_bound
increment internal version to 2

elasticmachine · 2019-08-09T12:36:25Z

Pinging @elastic/ml-core

…mestamp, checkpoint::time_upper_bound and increment version

benwtrent · 2019-08-09T17:15:22Z

bwc tests are failing, this is interesting as 7.4 transforms are being put, but then they cannot be found. Looking into it.

benwtrent · 2019-08-09T18:17:57Z

Ah, I see why bwc tests are failing.

TransportGetDataFrameTransformsAction does not take into account multiple indices. Need some refactoring

hendrikmuhs · 2019-08-09T18:27:17Z

I forgot that there is access outside of the config manager. The change I pushed might help but isn't enough. We need the same de-duplication as in the config manager to do this properly.

benwtrent · 2019-08-09T18:42:42Z

@hendrikmuhs I am fixing the bug now :)

…source with a given id

benwtrent · 2019-08-09T19:02:53Z

...data-frame/src/main/java/org/elasticsearch/xpack/dataframe/DataFrameInfoTransportAction.java

@@ -110,7 +110,7 @@ static void getStatisticSummations(Client client, ActionListener<DataFrameIndexe
            .filter(QueryBuilders.termQuery(DataFrameField.INDEX_DOC_TYPE.getPreferredName(),
                    DataFrameTransformStoredDoc.NAME)));

-        SearchRequestBuilder requestBuilder = client.prepareSearch(DataFrameInternalIndex.INDEX_NAME)
+        SearchRequestBuilder requestBuilder = client.prepareSearch(DataFrameInternalIndex.LATEST_INDEX_NAME)


this will not provide accurate data. Stats documents for various transforms could exist between various index versions. Since we use aggregations and a transform could have multiple stored docs between index versions, I am not sure we can even provide accurate stats.

@droberts195 What do you think?

This and DataFrameUsageTransportAction.java, these don't seem like critical pieces data and SOME information drift is OK. How we tackle this seems predicated on how we deal with old indices and how quickly they are cleaned up.

benwtrent · 2019-08-09T19:06:41Z

...ata-frame/src/main/java/org/elasticsearch/xpack/dataframe/DataFrameUsageTransportAction.java

@@ -114,7 +114,7 @@ protected void masterOperation(Task task, XPackUsageRequest request, ClusterStat
            }
        );

-        SearchRequest totalTransformCount = client.prepareSearch(DataFrameInternalIndex.INDEX_NAME)
+        SearchRequest totalTransformCount = client.prepareSearch(DataFrameInternalIndex.LATEST_INDEX_NAME)


What if a transform exists in the old index and has never been updated (thus NOT in the latest index) ?

...ain/java/org/elasticsearch/xpack/dataframe/persistence/DataFrameTransformsConfigManager.java

.../org/elasticsearch/xpack/dataframe/transforms/DataFrameTransformPersistentTasksExecutor.java

...e/src/main/java/org/elasticsearch/xpack/core/action/AbstractTransportGetResourcesAction.java

...n/java/org/elasticsearch/xpack/dataframe/action/TransportUpdateDataFrameTransformAction.java

davidkyle · 2019-08-19T17:06:21Z

...st/java/org/elasticsearch/xpack/dataframe/integration/DataFrameTransformInternalIndexIT.java

+    public void testUpdateDeletesOldTransformConfig() throws Exception {
+        TestRestHighLevelClient client = new TestRestHighLevelClient();
+        String transformIndex = "transform-index-deletes-old";
+        createSourceIndex(transformIndex);


Do you need to create the source index when you PUT directly to the index bypassing any validations or this for the update?

davidkyle · 2019-08-19T17:12:52Z

...rame/src/main/java/org/elasticsearch/xpack/dataframe/persistence/DataFrameInternalIndex.java

@@ -133,6 +145,8 @@ private static XContentBuilder mappings() throws IOException {
        addDataFrameTransformsConfigMappings(builder);
        // add the schema for transform stats
        addDataFrameTransformStoredDocMappings(builder);
+        // add the schema for checkpoints
+        addDataFrameCheckpointMappings(builder);


davidkyle · 2019-08-19T18:01:22Z

...frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameTransformTask.java

@@ -834,7 +835,18 @@ protected void doSaveState(IndexerState indexerState, DataFrameIndexerPosition p
                                    onStop();
                                    transformTask.shutdown();
                                }
-                                next.run();
+                                transformsConfigManager.deleteOldTransformStoredDocuments(transformId, ActionListener.wrap(


Will this be called everytime doSaveState is called? Is it possible to add something to the response to say whether or not the deletion is necessary

davidkyle · 2019-08-19T18:03:13Z

...ain/java/org/elasticsearch/xpack/dataframe/persistence/DataFrameTransformsConfigManager.java

+     * @param transformId The configuration ID potentially referencing configurations stored in the old indices
+     * @param listener listener to alert on completion
+     */
+    public void deleteOldTransformConfigurations(String transformId, ActionListener<Void> listener) {


Please add a test to DataFrameTransformsConfigManagerTests

davidkyle

LGTM

davidkyle · 2019-08-20T09:01:17Z

...frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameTransformTask.java

@@ -834,7 +837,23 @@ protected void doSaveState(IndexerState indexerState, DataFrameIndexerPosition p
                                    onStop();
                                    transformTask.shutdown();
                                }
-                                next.run();
+                                // Only do this clean up once, if it succeeded, no reason to do the query again.
+                                if (oldStatsCleanedUp.compareAndExchange(false, true) == false) {


nit: compareAndSet is cleaner as you don't need the negation.

if (oldStatsCleanedUp.compareAndSet(false, true) == false)

przemekwitek

LGTM

benwtrent · 2019-08-21T21:07:15Z

@elasticmachine update branch

benwtrent · 2019-08-21T22:13:38Z

run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro
run elasticsearch-ci/1

benwtrent · 2019-08-22T00:57:12Z

@elasticmachine update branch

benwtrent · 2019-08-22T12:22:26Z

run elasticsearch-ci/1

…5375) Adds index versioning for the internal data frame transform index. Allows for new indices to be created and referenced, `GET` requests now query over the index pattern and takes the latest doc (based on INDEX name).

…45837) Adds index versioning for the internal data frame transform index. Allows for new indices to be created and referenced, `GET` requests now query over the index pattern and takes the latest doc (based on INDEX name).

version data frame transform internal index

e67a1aa

hendrikmuhs added >enhancement WIP team-discuss :ml/Transform Transform labels Aug 9, 2019

Hendrik Muhs added 2 commits August 9, 2019 15:37

handle move on update, add some comments

0ee1be3

mention templates as well

1f4e500

hendrikmuhs marked this pull request as ready for review August 9, 2019 15:07

add mappings for config::version, config::create_time, checkpoint::ti…

9975df7

…mestamp, checkpoint::time_upper_bound and increment version

hendrikmuhs changed the title ~~[ML-DataFrame] POC: version data frame transform internal index~~ [ML-DataFrame] version data frame transform internal index Aug 9, 2019

hendrikmuhs added v7.4.0 v8.0.0 and removed WIP team-discuss labels Aug 9, 2019

fix style

446251b

benwtrent self-requested a review August 9, 2019 16:04

use the pattern for search instead of the last versioned index name

2726149

Adjusting abstract transport get resources to handle more than one re…

c62c602

…source with a given id

benwtrent reviewed Aug 9, 2019

View reviewed changes

benwtrent added 3 commits August 9, 2019 14:32

minor cleanup

fb4b4f3

Merge branch 'master' into HEAD

a6e28d6

running dbq to remove old documents for _update and new stats index

d8aea65

przemekwitek reviewed Aug 19, 2019

View reviewed changes

...e/src/main/java/org/elasticsearch/xpack/core/action/AbstractTransportGetResourcesAction.java Outdated Show resolved Hide resolved

...n/java/org/elasticsearch/xpack/dataframe/action/TransportUpdateDataFrameTransformAction.java Outdated Show resolved Hide resolved

addressing PR comments

8a6ea02

davidkyle reviewed Aug 19, 2019

View reviewed changes

addressing PR comments

49a2e07

davidkyle approved these changes Aug 20, 2019

View reviewed changes

benwtrent requested a review from przemekwitek August 20, 2019 12:21

przemekwitek reviewed Aug 20, 2019

View reviewed changes

Merge branch 'master' into df-index-refactor

a91d89d

Merge branch 'master' into df-index-refactor

b7fa3d9

benwtrent merged commit f989beb into elastic:master Aug 22, 2019

benwtrent mentioned this pull request Aug 22, 2019

[ML-DataFrame] version data frame transform internal index (#45375) #45837

Merged

benwtrent added >refactoring and removed >enhancement labels Aug 22, 2019

benwtrent mentioned this pull request Aug 22, 2019

[ML][Transforms] fix doSaveState check #45882

Merged

hendrikmuhs mentioned this pull request Oct 14, 2019

[Transform] transform jobs on nodes < 7.4.0 mixed with nodes > 7.4 might fail #48019

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML-DataFrame] version data frame transform internal index #45375

[ML-DataFrame] version data frame transform internal index #45375

hendrikmuhs commented Aug 9, 2019 •

edited

Loading

elasticmachine commented Aug 9, 2019

benwtrent commented Aug 9, 2019

benwtrent commented Aug 9, 2019

hendrikmuhs commented Aug 9, 2019

benwtrent commented Aug 9, 2019

benwtrent Aug 9, 2019

benwtrent Aug 9, 2019 •

edited

Loading

benwtrent Aug 9, 2019

davidkyle Aug 19, 2019

davidkyle Aug 19, 2019

davidkyle Aug 19, 2019

davidkyle Aug 19, 2019

davidkyle left a comment

davidkyle Aug 20, 2019

przemekwitek left a comment

benwtrent commented Aug 21, 2019

benwtrent commented Aug 21, 2019

benwtrent commented Aug 22, 2019

benwtrent commented Aug 22, 2019

[ML-DataFrame] version data frame transform internal index #45375

[ML-DataFrame] version data frame transform internal index #45375

Conversation

hendrikmuhs commented Aug 9, 2019 • edited Loading

elasticmachine commented Aug 9, 2019

benwtrent commented Aug 9, 2019

benwtrent commented Aug 9, 2019

hendrikmuhs commented Aug 9, 2019

benwtrent commented Aug 9, 2019

benwtrent Aug 9, 2019

Choose a reason for hiding this comment

benwtrent Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

benwtrent Aug 9, 2019

Choose a reason for hiding this comment

davidkyle Aug 19, 2019

Choose a reason for hiding this comment

davidkyle Aug 19, 2019

Choose a reason for hiding this comment

davidkyle Aug 19, 2019

Choose a reason for hiding this comment

davidkyle Aug 19, 2019

Choose a reason for hiding this comment

davidkyle left a comment

Choose a reason for hiding this comment

davidkyle Aug 20, 2019

Choose a reason for hiding this comment

przemekwitek left a comment

Choose a reason for hiding this comment

benwtrent commented Aug 21, 2019

benwtrent commented Aug 21, 2019

benwtrent commented Aug 22, 2019

benwtrent commented Aug 22, 2019

hendrikmuhs commented Aug 9, 2019 •

edited

Loading

benwtrent Aug 9, 2019 •

edited

Loading