Skip to content
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

Fix flaky test addSnapshotLifecyclePolicy #46881

Merged
merged 5 commits into from
Sep 20, 2019
Merged

Fix flaky test addSnapshotLifecyclePolicy #46881

merged 5 commits into from
Sep 20, 2019

Conversation

andreidan
Copy link
Contributor

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 Index and Snapshot lifecycle management 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
Collaborator

Pinging @elastic/es-core-features

dakrone
dakrone requested changes Sep 19, 2019
View reviewed changes
Copy link
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 Outdated
@@ -797,32 +797,11 @@ public void testAddSnapshotLifecyclePolicy() throws Exception {
// end::slm-put-snapshot-lifecycle-policy-response
assertTrue(putAcknowledged);

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

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 Outdated
@@ -858,18 +838,7 @@ public void onFailure(Exception e) {
SnapshotLifecyclePolicyMetadata policyMeta =
getResponse.getPolicies().get("policy_id"); // <1>
long policyVersion = policyMeta.getVersion();
long policyModificationDate = policyMeta.getModifiedDate();
Copy link
Member

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 Outdated
// end::slm-put-snapshot-lifecycle-policy-execute-listener

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

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
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
Contributor Author

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 <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
@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 Index and Snapshot lifecycle management >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