Skip to content

Fix flaky test addSnapshotLifecyclePolicy#46881

Merged
andreidan merged 5 commits intoelastic:masterfrom
andreidan:fix-flaky-testAddSnapshotLifecyclePolicy
Sep 20, 2019
Merged

Fix flaky test addSnapshotLifecyclePolicy#46881
andreidan merged 5 commits intoelastic:masterfrom
andreidan:fix-flaky-testAddSnapshotLifecyclePolicy

Conversation

@andreidan
Copy link
Copy Markdown
Contributor

@andreidan andreidan commented Sep 19, 2019

Fixes #46021

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@andreidan
testAddSnapshotLifecyclePolicy create policy once
32fa9d7 
Drop async call to put the policy as we are not waiting for the result
as we are synchronously creating it already (the async nature of the
second create request made this test flaky as we're asserting the
version of the policy to be 1)
@andreidan
testAddSnapshotLifecyclePolicy drop unused variables
af42449
@andreidan andreidan added >test Issues or PRs that are addressing/adding tests :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. v8.0.0 v7.5.0 labels Sep 19, 2019
@andreidan andreidan requested a review from dakrone September 19, 2019 16:14
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 19, 2019

Pinging @elastic/es-core-features

dakrone
dakrone requested changes Sep 19, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this Andrei, I left comments about what these unused blocks of code are for and why we shouldn't remove them.

...rest-high-level/src/test/java/org/elasticsearch/client/documentation/ILMDocumentationIT.java
assertTrue(putAcknowledged);

// tag::slm-put-snapshot-lifecycle-policy-execute-listener
ActionListener<AcknowledgedResponse> putListener =
Copy link
Copy Markdown
Member

@dakrone dakrone Sep 19, 2019

Choose a reason for hiding this comment

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

These are here because they are used as snippets to generate the high level rest client documentation (that's what the surrounding // tag::etcetc and // end::etcetc are for, so I don't think we should remove these

...rest-high-level/src/test/java/org/elasticsearch/client/documentation/ILMDocumentationIT.java
SnapshotLifecyclePolicyMetadata policyMeta =
getResponse.getPolicies().get("policy_id"); // <1>
long policyVersion = policyMeta.getVersion();
long policyModificationDate = policyMeta.getModifiedDate();
Copy link
Copy Markdown
Member

@dakrone dakrone Sep 19, 2019

Choose a reason for hiding this comment

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

Even though these appear unused, they are documenting how to retrieve things from the policy metadata, so we should leave them. See get_snapshot_lifecycle_policy.asciidoc for the callout usages

...rest-high-level/src/test/java/org/elasticsearch/client/documentation/ILMDocumentationIT.java
// end::slm-put-snapshot-lifecycle-policy-execute-listener

// tag::slm-put-snapshot-lifecycle-policy-execute-async
client.indexLifecycle().putSnapshotLifecyclePolicyAsync(request,
Copy link
Copy Markdown
Member

@dakrone dakrone Sep 19, 2019

Choose a reason for hiding this comment

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

I think rather than remove this (which we need for documentation), we should relax or remove the assertion to not trip on the policy version.

@andreidan
Revert "testAddSnapshotLifecyclePolicy drop unused variables"
19b6dbd 
This reverts commit af42449.
@andreidan
Revert "testAddSnapshotLifecyclePolicy create policy once"
004b126 
This reverts commit 32fa9d7.
@andreidan
addSnapshotLifecyclePolicy drop version assertion
f07f8a0 
This drops the assertion on the policy version (which was pinned to 1L)
as we want to execute both put policy apis (sync and async) for
documentation purposes. This will sometime (depending on the async
call) yield a version of 2L. Waiting for the async call to always
complete could be an option but the test is already rather slow and it's
a bit of an overkill as we're already verifyin the policy was created.
@andreidan andreidan requested a review from dakrone September 19, 2019 16:52
dakrone
dakrone approved these changes Sep 19, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

@dakrone dakrone added the v7.4.1 label Sep 19, 2019
@andreidan
Copy link
Copy Markdown
Contributor Author

andreidan commented Sep 19, 2019

retest this please

@andreidan andreidan merged commit af4864c into elastic:master Sep 20, 2019
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Sep 20, 2019
@andreidan
Fix flaky test addSnapshotLifecyclePolicy (elastic#46881)
b97ba1c 
* addSnapshotLifecyclePolicy drop version assertion

This drops the assertion on the policy version (which was pinned to 1L)
as we want to execute both put policy apis (sync and async) for
documentation purposes. This will sometimes (depending on the async
call) yield a version of 2L. Waiting for the async call to always
complete could be an option but the test is already rather slow and it's
a bit of an overkill as we're already verifying the policy was created.

(cherry picked from commit af4864c)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
@andreidan andreidan mentioned this pull request Sep 20, 2019
Merged
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Sep 20, 2019
@andreidan
Fix flaky test addSnapshotLifecyclePolicy (elastic#46881)
a3af247 
* addSnapshotLifecyclePolicy drop version assertion

This drops the assertion on the policy version (which was pinned to 1L)
as we want to execute both put policy apis (sync and async) for
documentation purposes. This will sometimes (depending on the async
call) yield a version of 2L. Waiting for the async call to always
complete could be an option but the test is already rather slow and it's
a bit of an overkill as we're already verifying the policy was created.

(cherry picked from commit af4864c)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
@andreidan andreidan mentioned this pull request Sep 20, 2019
Merged
andreidan added a commit that referenced this pull request Sep 20, 2019
@andreidan
Fix flaky test addSnapshotLifecyclePolicy (#46881) (#46912)
402fb5e 
* addSnapshotLifecyclePolicy drop version assertion

This drops the assertion on the policy version (which was pinned to 1L)
as we want to execute both put policy apis (sync and async) for
documentation purposes. This will sometimes (depending on the async
call) yield a version of 2L. Waiting for the async call to always
complete could be an option but the test is already rather slow and it's
a bit of an overkill as we're already verifying the policy was created.

(cherry picked from commit af4864c)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
@andreidan andreidan deleted the fix-flaky-testAddSnapshotLifecyclePolicy branch September 20, 2019 16:02
andreidan added a commit that referenced this pull request Sep 20, 2019
@andreidan
Fix flaky test addSnapshotLifecyclePolicy (#46881) (#46915)
28e06d9 
* addSnapshotLifecyclePolicy drop version assertion

This drops the assertion on the policy version (which was pinned to 1L)
as we want to execute both put policy apis (sync and async) for
documentation purposes. This will sometimes (depending on the async
call) yield a version of 2L. Waiting for the async call to always
complete could be an option but the test is already rather slow and it's
a bit of an overkill as we're already verifying the policy was created.

(cherry picked from commit af4864c)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
@colings86 colings86 added v7.4.0 and removed v7.4.1 labels Sep 25, 2019
@dnhatn dnhatn mentioned this pull request Oct 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dakrone dakrone dakrone approved these changes

Assignees

No one assigned

Labels

:Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. >test Issues or PRs that are addressing/adding tests v7.4.0 v7.5.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] Failure in ILMDocumentationIT.testAddSnapshotLifecyclePolicy

5 participants

@andreidan @elasticmachine @dakrone @colings86 @jakelandis