Make testSortAndPaginateWithInProgress test stable by arteam · Pull Request #80530 · elastic/elasticsearch

arteam · 2021-11-09T12:47:32Z

Start asserting snapshots in progress only in case when they reach
a stable state (the first index has finished, the second has been
blocked).

Fixes #79779
Relates #78507

Start asserting snapshots in progress only in case when they reach a stable state (the first index has finished, the second has been blocked). Fixes elastic#79779 Relates elastic#78507

elasticmachine · 2021-11-09T12:50:28Z

Pinging @elastic/es-distributed (Team:Distributed)

arteam · 2021-11-11T10:18:21Z

@elasticmachine update branch

arteam · 2021-11-25T15:56:56Z

@elasticmachine update branch

henningandersen

Left a couple of comments to ensure I understand the change correctly.

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

henningandersen · 2021-11-29T10:50:27Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

+        .put("thread_pool.snapshot.core", 5)
+        .put("thread_pool.snapshot.max", 5)


Can you add a comment on why this need to be raised?

Also, I think 2 would be enough, 1 to be blocked for test-index-2 and one to ensure test-index-1 completes?

I think you missed this comment?

Sorry, I indeed missed it! I've moved LARGE_SNAPSHOT_POOL_SETTINGS to RestGetSnapshotsIT. I believe 2 is insufficient, the test hangs with 2 threads, but works with 3.

…used

original-brownbear

I don't think this is enough unfortunately, see inline comment.

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

arteam · 2021-12-15T17:33:31Z

@elasticmachine update branch

arteam · 2022-01-05T14:26:01Z

@elasticmachine update branch

original-brownbear

I think the CS wait check can be simplified some more, other than that looks alright.

original-brownbear · 2022-01-11T11:17:22Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

+                    e -> e.getKey().getIndexName().equals("test-index-1") == false
+                        || e.getValue().state() == SnapshotsInProgress.ShardState.SUCCESS
+                );
+            boolean secondIndexIsBlocked = state.custom(SnapshotsInProgress.TYPE, SnapshotsInProgress.EMPTY)


We shouldn't need this second check. If we make sure that all snapshots for index-1 are successful, then that should be the end state of things. The state of snapshots for the second index should be initialized to exactly what we're asserting here right away I think?

#80530 (comment)

@original-brownbear That was the original implementation (with only the test-index-1 check) and you suggested to add an addition check for the second index 😃

I don't have a strong feeling towards this one way or another (the test passes on 100 iterations both with secondIndexIsBlocked check and without it).

Right, now I remember :) Sorry about that.

arteam · 2022-01-13T11:30:29Z

@elasticmachine update branch

arteam · 2022-01-13T11:44:38Z

@elasticmachine run elasticsearch-ci/eql-correctness

arteam · 2022-01-18T17:45:03Z

@elasticmachine update

original-brownbear

LGTM, sorry for the delay here

original-brownbear · 2022-01-18T18:15:29Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

+                    e -> e.getKey().getIndexName().equals("test-index-1") == false
+                        || e.getValue().state() == SnapshotsInProgress.ShardState.SUCCESS
+                );
+            boolean secondIndexIsBlocked = state.custom(SnapshotsInProgress.TYPE, SnapshotsInProgress.EMPTY)


Right, now I remember :) Sorry about that.

arteam · 2022-01-18T18:16:33Z

Thanks @original-brownbear!

Start asserting snapshots in progress only in case when they reach a stable state (the first index has finished, the second has been blocked). * Move LARGE_SNAPSHOT_SETTINGS to AbstractSnapshotRestTestCase to be reused * Check that test-index-2 is blocked * Be more clear that the 2nd index is blocked Fixes elastic#79779 Relates elastic#78507

elasticsearchmachine · 2022-01-18T18:17:41Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.17

Start asserting snapshots in progress only in case when they reach a stable state (the first index has finished, the second has been blocked). * Move LARGE_SNAPSHOT_SETTINGS to AbstractSnapshotRestTestCase to be reused * Check that test-index-2 is blocked * Be more clear that the 2nd index is blocked Fixes #79779 Relates #78507

…82759) Start asserting snapshots in progress only in case when they reach a stable state (the first index has finished, the second has been blocked). * Move LARGE_SNAPSHOT_SETTINGS to AbstractSnapshotRestTestCase to be reused * Check that test-index-2 is blocked * Be more clear that the 2nd index is blocked Fixes #79779 Relates #78507

Make testSortAndPaginateWithInProgress test stable

cd303f3

Start asserting snapshots in progress only in case when they reach a stable state (the first index has finished, the second has been blocked). Fixes elastic#79779 Relates elastic#78507

elasticsearchmachine added the v8.1.0 label Nov 9, 2021

arteam added the :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Nov 9, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Nov 9, 2021

arteam added v7.16.1 auto-backport Automatically create backport pull requests when merged >test Issues or PRs that are addressing/adding tests and removed Team:Distributed Meta label for distributed team. labels Nov 9, 2021

original-brownbear self-requested a review November 9, 2021 13:05

elasticmachine added 3 commits November 11, 2021 02:18

Merge branch 'master' into fix-test-sort-paginate-with-in-progress

46ec9fa

Merge branch 'master' into fix-test-sort-paginate-with-in-progress

c297497

Merge branch 'master' into fix-test-sort-paginate-with-in-progress

dd760a4

Merge branch 'master' into fix-test-sort-paginate-with-in-progress

5d27d41

arteam closed this Nov 25, 2021

arteam reopened this Nov 25, 2021

arteam requested review from henningandersen and original-brownbear and removed request for original-brownbear November 29, 2021 09:02

henningandersen reviewed Nov 29, 2021

View reviewed changes

Move LARGE_SNAPSHOT_SETTINGS to AbstractSnapshotRestTestCase to be re…

05830e2

…used

original-brownbear suggested changes Nov 29, 2021

View reviewed changes

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java Outdated Show resolved Hide resolved

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java Outdated Show resolved Hide resolved

Check that test-index-2 is blocked

6a43212

arteam requested review from henningandersen and original-brownbear November 29, 2021 16:05

arteam added v7.16.2 and removed v7.16.1 labels Dec 13, 2021

mark-vieira added v7.16.3 and removed v7.16.2 labels Dec 18, 2021

elasticmachine and others added 2 commits January 5, 2022 07:26

Merge branch 'master' into fix-test-sort-paginate-with-in-progress

8594b00

Be more clear that the 2nd index is blocked

1b42bfa

arteam requested a review from henningandersen January 6, 2022 20:51

bpintea added v7.16.4 and removed v7.16.3 labels Jan 10, 2022

original-brownbear reviewed Jan 11, 2022

View reviewed changes

Merge branch 'master' into fix-test-sort-paginate-with-in-progress

a1390fc

arteam requested a review from original-brownbear January 13, 2022 16:34

arteam added v8.0.0 and removed v7.16.4 labels Jan 14, 2022

original-brownbear approved these changes Jan 18, 2022

View reviewed changes

arteam merged commit b5d83a5 into elastic:master Jan 18, 2022

arteam mentioned this pull request Jan 18, 2022

[8.0] Make testSortAndPaginateWithInProgress test stable (#80530) #82758

Merged

arteam mentioned this pull request Jan 18, 2022

[7.17] Make testSortAndPaginateWithInProgress test stable (#80530) #82759

Merged

arteam deleted the fix-test-sort-paginate-with-in-progress branch January 18, 2022 18:23

pugnascotia added v8.0.0-rc2 and removed v8.0.0 labels Feb 1, 2022

		.put("thread_pool.snapshot.core", 5)
		.put("thread_pool.snapshot.max", 5)

Conversation

arteam commented Nov 9, 2021

Uh oh!

elasticmachine commented Nov 9, 2021

Uh oh!

arteam commented Nov 11, 2021

Uh oh!

arteam commented Nov 25, 2021

Uh oh!

henningandersen left a comment

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!

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arteam commented Dec 15, 2021

Uh oh!

arteam commented Jan 5, 2022

Uh oh!

original-brownbear 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arteam commented Jan 13, 2022

Uh oh!

arteam commented Jan 13, 2022

Uh oh!

arteam commented Jan 18, 2022

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arteam commented Jan 18, 2022

Uh oh!

elasticsearchmachine commented Jan 18, 2022

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants