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.

Sign up for GitHub

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

Jump to bottom

[Transform] Improve robustness when saving state #62086

Merged

hendrikmuhs merged 21 commits into elastic:master from hendrikmuhs:transform-saveState

Sep 28, 2020

hendrikmuhs commented Sep 8, 2020 •

edited

Loading

refactor how state is persisted, call doSaveState only from the indexer thread, except there is none.

fixes #60781
fixes #52931
fixes #51629
fixes #52035

hendrikmuhs added >test >refactoring v8.0.0 :ml/Transform v7.10.0 labels

hendrikmuhs marked this pull request as ready for review

September 10, 2020 13:47

Collaborator

elasticmachine commented Sep 10, 2020

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

hendrikmuhs force-pushed the transform-saveState branch 3 times, most recently from 37e71c2 to 6f77dac Compare

September 14, 2020 07:45

benwtrent self-requested a review

September 14, 2020 11:33

przemekwitek reviewed

View reviewed changes

.../javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformIntegTestCase.java Outdated Show resolved Hide resolved

.../javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformIntegTestCase.java Outdated Show resolved Hide resolved

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexing/AsyncTwoPhaseIndexer.java Outdated Show resolved Hide resolved

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexing/AsyncTwoPhaseIndexer.java Outdated Show resolved Hide resolved

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexing/AsyncTwoPhaseIndexer.java Outdated Show resolved Hide resolved

benwtrent reviewed

View reviewed changes

...ugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformTask.java Outdated Show resolved Hide resolved

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java Outdated Show resolved Hide resolved

Author

hendrikmuhs commented Sep 15, 2020

Thanks @przemekwitek and @benwtrent, I addressed your comments.

przemekwitek approved these changes

View reviewed changes

Contributor

przemekwitek left a comment

LGTM

I've left one small comment but there is no need for another review.

.../javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformIntegTestCase.java Outdated Show resolved Hide resolved

Author

hendrikmuhs commented Sep 15, 2020

run elasticsearch-ci/bwc

hendrikmuhs force-pushed the transform-saveState branch from d0aa4ee to 883ee76 Compare

September 18, 2020 15:22

Author

hendrikmuhs commented Sep 21, 2020

run elasticsearch-ci/2

benwtrent reviewed

View reviewed changes

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java Outdated Show resolved Hide resolved

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java Outdated Show resolved Hide resolved

hendrikmuhs force-pushed the transform-saveState branch from 36f5ef4 to 5a475ad Compare

September 21, 2020 19:23

Author

hendrikmuhs commented Sep 22, 2020

run elasticsearch-ci/packaging-sample-windows

benwtrent approved these changes

View reviewed changes

Member

benwtrent left a comment

I think all the branches are covered.

Not a huge fan of a latch away within a sync block. But, this distributed synchronizing of 3 separate mutable state values is getting out of hand.

Our testing should catch any weird conditions over time.

Author

hendrikmuhs commented Sep 22, 2020

run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro
run elasticsearch-ci/packaging-sample-windows

hendrikmuhs force-pushed the transform-saveState branch from 21aa065 to 5a52149 Compare

September 22, 2020 15:20

Author

hendrikmuhs commented Sep 23, 2020

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

hendrikmuhs force-pushed the transform-saveState branch from 5a52149 to a7df454 Compare

September 23, 2020 06:12

Hendrik Muhs added 4 commits

September 23, 2020 12:24


          refactor how state is persisted, so it can not be called from more th…

25dfb54

…an 1

thread at the same time by moving it completely into the indexer.


          unmute integration tests

196b5c1


          enhance the integration test to do several runs with start and stop

926d683


          apply review comments

269f615

Hendrik Muhs added 12 commits

September 23, 2020 12:24


          remove superflous line

8ce27e5


          Call listener immediately if indexer is not indexing or value hasn't …

ca5e7b2

…changed.

Use optimistic lock for listeners


          add module tests for shopAtCheckpoint

2364f74


          add file headers

56388b2


          persist state if indexer state is started

bf1ab27


          log if state persistance fails

139ba31


          do not block the network thread

b092ac6


          adapt tests, so it does not assert on the threadpool


          add testcase for throttling

cb7829c


          assert that state has been persisted with shouldStopAtCheckpoint

447504b


          ensure listeners are called if transform stops

4eb2b53


          add back synchronized to setShouldStopAtCheckpoint to avoid problems

3b06daf

if start runs in parralel

hendrikmuhs force-pushed the transform-saveState branch from a7df454 to 3b06daf Compare

September 23, 2020 10:25

Author

hendrikmuhs commented Sep 23, 2020

run elasticsearch-ci/2

Hendrik Muhs added 4 commits

September 23, 2020 17:32


          do not call action listener onResponse inside getAndUpdate

1687e32


          simplify by using updateAndGet

38994fb


          make updateAndGet free of side effects

552023a


          do not call listener from synchronized block

d0efdfd

Author

hendrikmuhs commented Sep 23, 2020

run elasticsearch-ci/1
run elasticsearch-ci/2

3 similar comments

Author

hendrikmuhs commented Sep 24, 2020

run elasticsearch-ci/1
run elasticsearch-ci/2

Author

hendrikmuhs commented Sep 24, 2020

run elasticsearch-ci/1
run elasticsearch-ci/2

Author

hendrikmuhs commented Sep 24, 2020

run elasticsearch-ci/1
run elasticsearch-ci/2

Author

hendrikmuhs commented Sep 24, 2020

run elasticsearch-ci/1
run elasticsearch-ci/2

(one last round :-) )

benwtrent approved these changes

View reviewed changes

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

+                   */
+                  final void setStopAtCheckpoint(boolean shouldStopAtCheckpoint, ActionListener<Void> shouldStopAtCheckpointListener) {
+                      // this should be called from the generic threadpool
+                      assert Thread.currentThread().getName().contains(ThreadPool.Names.GENERIC);

Member

benwtrent Sep 24, 2020

In testing (or where assertions are enabled), this would cause the caller to freeze and never return.

I am not sure how to assert here AND fire an onFailure result. But, not doing so might cause some frustrating test investigations

hendrikmuhs merged commit 3fc407b into elastic:master

hendrikmuhs deleted the transform-saveState branch

September 28, 2020 06:53

This was referenced Sep 28, 2020

[7.x][Transform] Improve robustness when saving state #62927

Merged

[Transform] Workflow testing of continuous/batch data frame transforms #45369

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:ml/Transform >refactoring >test v7.10.0 v8.0.0-alpha1