[Transform] implement retention policy to delete data from a transform #67832

hendrikmuhs · 2021-01-21T15:43:27Z

add a retention policy to transform to delete data that is considered outdated as part of a transform checkpoint.

fixes #67916

todo:

documentation changes
support for added counters in _cat/transforms/

mark-vieira · 2021-02-03T00:16:13Z

@elasticmachine update branch

hendrikmuhs · 2021-02-03T17:31:16Z

run elasticsearch-ci/default-distro

elasticmachine · 2021-02-04T07:34:49Z

Pinging @elastic/ml-core (:ml/Transform)

hendrikmuhs · 2021-02-04T14:31:38Z

.../java/org/elasticsearch/xpack/transform/transforms/TransformIndexerFailureHandlingTests.java

+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+public class TransformIndexerFailureHandlingTests extends ESTestCase {


this is the renamed TransformIndexerTests, no changes done

hendrikmuhs · 2021-02-04T14:33:05Z

...nsform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformIndexerTests.java

 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;

 public class TransformIndexerTests extends ESTestCase {


I moved the former TransformIndexerTests to TransformIndexerFailureHandlingTests, so this is a new file.

...-high-level/src/main/java/org/elasticsearch/client/transform/transforms/TransformConfig.java

server/src/main/java/org/elasticsearch/action/admin/indices/refresh/RefreshResponse.java

...in/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java

...e/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigUpdate.java

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java

przemekwitek

Partial review, will finish in the afternoon.

I wish renaming the unit test base classes was a separate PR but no worries, I'll be able to go through it.

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

...e/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformIndexerStats.java

...re/src/test/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigTests.java

...rc/test/java/org/elasticsearch/xpack/core/transform/transforms/latest/LatestConfigTests.java

...sform/src/main/java/org/elasticsearch/xpack/transform/transforms/ClientTransformIndexer.java

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformContext.java

...elasticsearch/xpack/transform/transforms/RetentionPolicyToDeleteByQueryRequestConverter.java

przemekwitek · 2021-02-05T10:10:11Z

...elasticsearch/xpack/transform/transforms/RetentionPolicyToDeleteByQueryRequestConverter.java

+        if (retentionPolicyConfig instanceof TimeRetentionPolicyConfig) {
+            request.setQuery(getDeleteQueryFromTimeBasedRetentionPolicy((TimeRetentionPolicyConfig) retentionPolicyConfig, checkpoint));
+        } else {
+            throw new RetentionPolicyException("unsupported retention policy of type [{}]", retentionPolicyConfig.getWriteableName());


Wouldn't this cause problems when other types of policies are added in subsequent versions?

You could add the accessors to RetentionPolicyConfig so that you don't have to do this casting.

Or RetentionPolicyConfig know everything about itself it could have a toDeleteByQueryRequest method

Yeah it would? I think thats the idea. If you add a new policy, you sure better make sure to write a request converter.

Throwing here is a good pattern. It enforces case handling.

Or RetentionPolicyConfig know everything about itself it could have a toDeleteByQueryRequest method

Intentionally I avoided that. We had (probably still have) a lot of logic in the config classes in the past and I had a hard time to separate config and logic.

I agree it looks a little dumb at the moment, its just 1 config with 1 implementation.

benwtrent

👍 from me.

I really like how nicely it falls into the indexer finish flow.

przemekwitek · 2021-02-05T15:17:10Z

client/rest-high-level/src/test/java/org/elasticsearch/client/TransformIT.java

-                .endObject()
-                .endObject()
-                .endObject();
+            .startObject("properties")


Is there any integration test added for the new functionality? I couldn't find it in this PR and I think it would be beneficial.

I had a hard time to craft one, I don't want to slow down the test execution further. It would be nice to manipulate the clock, but I think that's hard to do from the outside. E.g. I defined that max_age must be at least 60s. So an integration test must run at least 60s.

What I did:

unit test: test a policy is translated to a delete by query

module test: test that the indexer is calling the delete extension points if a policy is there and not if a policy is null -> ensures the right hooks are called at the end of the checkpoint. see the new TransformIndexerTests

out of scope:

testing correctness of delete by query

davidkyle

LGTM

davidkyle · 2021-02-05T11:56:44Z

...elasticsearch/xpack/transform/transforms/RetentionPolicyToDeleteByQueryRequestConverter.java

+        if (retentionPolicyConfig instanceof TimeRetentionPolicyConfig) {
+            request.setQuery(getDeleteQueryFromTimeBasedRetentionPolicy((TimeRetentionPolicyConfig) retentionPolicyConfig, checkpoint));
+        } else {
+            throw new RetentionPolicyException("unsupported retention policy of type [{}]", retentionPolicyConfig.getWriteableName());


You could add the accessors to RetentionPolicyConfig so that you don't have to do this casting.

Or RetentionPolicyConfig know everything about itself it could have a toDeleteByQueryRequest method

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java

...sform/src/main/java/org/elasticsearch/xpack/transform/transforms/ClientTransformIndexer.java

hendrikmuhs · 2021-02-05T15:57:57Z

client/rest-high-level/src/test/java/org/elasticsearch/client/TransformIT.java

-                .endObject()
-                .endObject()
-                .endObject();
+            .startObject("properties")


I had a hard time to craft one, I don't want to slow down the test execution further. It would be nice to manipulate the clock, but I think that's hard to do from the outside. E.g. I defined that max_age must be at least 60s. So an integration test must run at least 60s.

What I did:

unit test: test a policy is translated to a delete by query

module test: test that the indexer is calling the delete extension points if a policy is there and not if a policy is null -> ensures the right hooks are called at the end of the checkpoint. see the new TransformIndexerTests

out of scope:

testing correctness of delete by query

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformContext.java

hendrikmuhs · 2021-02-05T16:48:06Z

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java

+            // paranoia: we are not expecting dbq to fail for other reasons
+            if (bulkByScrollResponse.getBulkFailures().size() > 0 || bulkByScrollResponse.getSearchFailures().size() > 0) {
+                assert false : "delete by query failed unexpectedly" + bulkByScrollResponse;
+                logger.warn(


I added this case, but actually think I will change it to call onFailure, it would be inconsistent to search and indexing errors, I mean that's why we have retry-logic.

hendrikmuhs · 2021-02-05T16:54:21Z

...elasticsearch/xpack/transform/transforms/RetentionPolicyToDeleteByQueryRequestConverter.java

+        if (retentionPolicyConfig instanceof TimeRetentionPolicyConfig) {
+            request.setQuery(getDeleteQueryFromTimeBasedRetentionPolicy((TimeRetentionPolicyConfig) retentionPolicyConfig, checkpoint));
+        } else {
+            throw new RetentionPolicyException("unsupported retention policy of type [{}]", retentionPolicyConfig.getWriteableName());


Or RetentionPolicyConfig know everything about itself it could have a toDeleteByQueryRequest method

Intentionally I avoided that. We had (probably still have) a lot of logic in the config classes in the past and I had a hard time to separate config and logic.

I agree it looks a little dumb at the moment, its just 1 config with 1 implementation.

… results

przemekwitek

LGTM

Just one nit on json formatting in Java code.

przemekwitek · 2021-02-08T12:03:13Z

...rc/test/java/org/elasticsearch/xpack/core/transform/transforms/latest/LatestConfigTests.java

-            + " \"unique_key\": [],"
-            + " \"sort\": \"timestamp\""
-            + "}";
+        String json = "{" + " \"unique_key\": []," + " \"sort\": \"timestamp\"" + "}";


Do you plan to apply

// tag::NO_CODE_FORMAT

here and in other test cases?
Now it looks strange IMO to have this concatenations in one line.

sorry, I did not fix those, will do.

szabosteve

Docs changes LGTM. Thanks!

elastic#67832) add a retention policy to transform to delete data that is considered outdated as part of a transform checkpoint. fixes elastic#67916

…nsform (#67832) (#68814) add a retention policy to transform to delete data that is considered outdated as part of a transform checkpoint. backport #67832 fixes #67916

…68884) finalize backport of #67832

add retention policy and sync to documentation IT, add builders for sync and retention policy relates #67832

elastic#67832 fixes elastic#51154

When using wait_for_checkpoint the destination index should be searchable when the call returns. This issue has been fixed as part of #67832. This change removes the index refresh workarounds still present. fixes #51154

hendrikmuhs added >enhancement v8.0.0 :ml/Transform Transform v7.12.0 labels Jan 21, 2021

hendrikmuhs force-pushed the transform-retention branch from 45b6960 to f46a155 Compare January 26, 2021 15:28

hendrikmuhs changed the title ~~[Transform] add the possibility to delete data based on a retention policy~~ [Transform] implement retention policy to delete data from a transform Jan 26, 2021

hendrikmuhs force-pushed the transform-retention branch 2 times, most recently from c75a88e to 029f091 Compare February 1, 2021 09:52

hendrikmuhs force-pushed the transform-retention branch from 9c36908 to 158f96b Compare February 3, 2021 09:36

hendrikmuhs force-pushed the transform-retention branch from ac758be to 1985bd3 Compare February 4, 2021 06:44

hendrikmuhs marked this pull request as ready for review February 4, 2021 07:34

hendrikmuhs commented Feb 4, 2021

View reviewed changes

benwtrent self-requested a review February 4, 2021 15:27

benwtrent reviewed Feb 4, 2021

View reviewed changes

przemekwitek reviewed Feb 5, 2021

View reviewed changes

benwtrent approved these changes Feb 5, 2021

View reviewed changes

przemekwitek reviewed Feb 5, 2021

View reviewed changes

davidkyle approved these changes Feb 5, 2021

View reviewed changes

hendrikmuhs commented Feb 5, 2021

View reviewed changes

hendrikmuhs force-pushed the transform-retention branch from 76f3dde to 8068051 Compare February 8, 2021 08:51

Hendrik Muhs added 7 commits February 8, 2021 10:34

create config classes for retention policy configuration

9118761

apply spotless

efd718c

fix license headers

a732dfd

add missing headers

35e7c9a

checkstyle

04bc8c8

fix compile error

59acf31

more compile errors

4daf884

fail (with retry) if delete by query fails or returns with unexpected…

2b5c051

… results

przemekwitek approved these changes Feb 8, 2021

View reviewed changes

fix code formating

90681b3

szabosteve approved these changes Feb 8, 2021

View reviewed changes

szabosteve mentioned this pull request Feb 8, 2021

[DOCS] Adds retention_policy to PUT Transform API docs #68656

Merged

hendrikmuhs merged commit 54ed2e3 into elastic:master Feb 8, 2021

hendrikmuhs deleted the transform-retention branch February 8, 2021 14:06

This was referenced Feb 9, 2021

[Transform] add retention policy to documentation IT #68728

Merged

[7.x][Transform] implement retention policy to delete data from a transform (#67832) #68814

Merged

hendrikmuhs mentioned this pull request Feb 11, 2021

[Transform] finalize backport of retention_policy and reenable BWC #68884

Merged

hendrikmuhs pushed a commit that referenced this pull request Feb 11, 2021

[Transform] finalize backport of retention_policy and reenable BWC (#…

91bcce9

…68884) finalize backport of #67832

hendrikmuhs pushed a commit that referenced this pull request Feb 11, 2021

[Transform] add retention policy to documentation IT (#68728)

967505c

add retention policy and sync to documentation IT, add builders for sync and retention policy relates #67832

hendrikmuhs pushed a commit that referenced this pull request Feb 11, 2021

[Transform] add retention policy to documentation IT (#68728)

a0bf315

add retention policy and sync to documentation IT, add builders for sync and retention policy relates #67832

stevejgordon mentioned this pull request Feb 22, 2021

7.12.0 Meta Ticket elastic/elasticsearch-net#5337

Closed

34 tasks

hendrikmuhs mentioned this pull request Mar 10, 2021

[Transform] wait for checkpoint does not refresh the dest index #51154

Closed

hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Mar 11, 2021

remove index refresh workarounds, the underlying issue has been fixed in

6584a5d

elastic#67832 fixes elastic#51154

hendrikmuhs mentioned this pull request Mar 11, 2021

[Transform] remove index refresh workardounds from test cases #70296

Merged

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

[Transform] implement retention policy to delete data from a transform #67832

[Transform] implement retention policy to delete data from a transform #67832

Uh oh!

Conversation

hendrikmuhs commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mark-vieira commented Feb 3, 2021

Uh oh!

hendrikmuhs commented Feb 3, 2021

Uh oh!

elasticmachine commented Feb 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

przemekwitek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benwtrent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkyle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

przemekwitek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szabosteve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hendrikmuhs commented Jan 21, 2021 •

edited

Loading