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.

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

Remove retention leases when unfollowing #39088

Merged
merged 11 commits into from Feb 20, 2019

Conversation

Projects
None yet
5 participants
@jasontedor
Copy link
Member

commented Feb 19, 2019

This commit attempts to remove the retention leases on the leader shards when unfollowing an index. This is best effort, since the leader might not be available.

Closes #34648
Relates #37165

Remove retention leases when unfollowing
This commit attempts to remove the retention leases on the leader shards
when unfollowing an index. This is best effort, since the leader might
not be available.
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2019

@jasontedor jasontedor referenced this pull request Feb 19, 2019

Closed

Shard history retention leases #37165

23 of 24 tasks complete
remoteClient,
ActionListener.wrap(
groupListener::onResponse,
// TODO: should this should exponentially backoff, or should we simply never retry?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Feb 19, 2019

Author Member

@martijnvg @dnhatn I opened this PR as WIP for discussion on this point. I am curious to hear your thoughts. Note that failure is a definite possibility since a common use case for the unfollow API will be when the leader is unavailable.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Feb 19, 2019

Member

I'm leaning towards not retrying. However I think that we should add a field in the response indication whether retention lease removal was successful. (retention_lease_removal boolean field?)

This comment has been minimized.

Copy link
@martijnvg

martijnvg Feb 19, 2019

Member

(my motivation for not retrying is, that because it is a common scenario and therefore retrying would be futile to begin with).

This comment has been minimized.

Copy link
@jasontedor

jasontedor Feb 19, 2019

Author Member

Yes, that's my inclination as well.

@dnhatn Is this okay with you too?

This comment has been minimized.

Copy link
@dnhatn

dnhatn Feb 19, 2019

Contributor

I am + 1 to "not retry here" because a retention lease will be automatically expired. We need to make sure ILM works well in this scenario.

jasontedor added some commits Feb 19, 2019

jasontedor added some commits Feb 20, 2019

wip
Merge remote-tracking branch 'elastic/master' into retention-lease-un…
…follow

* elastic/master: (37 commits)
  Enable test logging for TransformIntegrationTests#testSearchTransform.
  stronger wording for ilm+rollover in docs (#39159)
  Mute SingleNodeTests (#39156)
  AwaitsFix XPackUsageIT#testXPackCcrUsage.
  Resolve concurrency with watcher trigger service (#39092)
  Fix median calculation in MedianAbsoluteDeviationAggregatorTests (#38979)
  [DOCS] Edits the remote clusters documentation (#38996)
  add version 6.6.2
  Revert "Mute failing test 20_mix_typless_typefull (#38781)" (#38912)
  Rebuild remote connections on profile changes (#37678)
  Document 'max_size' parameter as shard size for rollover (#38750)
  Add some missing toString() implementations (#39124)
  Migrate Streamable to Writeable for cluster block package (#37391)
  fix RethrottleTests retry (#38978)
  Disable date parsing test in non english locale (#39052)
  Remove BCryptTests (#39098)
  [ML] Stop the ML memory tracker before closing node (#39111)
  Allow retention lease operations under blocks (#39089)
  ML refactor DatafeedsConfig(Update) so defaults are not populated in queries or aggs (#38822)
  Fix retention leases sync on recovery test
  ...
@dnhatn

dnhatn approved these changes Feb 20, 2019

Copy link
Contributor

left a comment

LGTM. Thanks @jasontedor.

retentionLeasesRemoved = in.readBoolean();
// noinspection StatementWithEmptyBody
if (retentionLeasesRemoved) {

This comment has been minimized.

Copy link
@dnhatn

dnhatn Feb 20, 2019

Contributor

Did you try to have a symmetric flow with writeTo?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Feb 20, 2019

Author Member

Yes, but now I have removed this code. 😇

jasontedor added some commits Feb 20, 2019

@jasontedor jasontedor removed the WIP label Feb 20, 2019

@jasontedor jasontedor requested a review from gwbrown Feb 20, 2019

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

@gwbrown @dnhatn @martijnvg I have removed the WIP label, this is ready for review.

@gwbrown @martijnvg I decided to not implement the case of not being able to remove the shards the way that you have proposed. I went a different way for two reasons:

  • I did not like having a different way to express failure than we normally express failure; either the request succeeded or it did not and I think we should not indicate certain kinds of failures (inability to remove retention leases) differently from others. Instead, we indicate a failure to remove the retention leases via metadata on a wrapper exception.
  • We would have to lift UnfollowAction.Response transport response to a shard dependency between CCR and ILM (like x-pack-core).

Please me know your thoughts and if this will suffice for ILM.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

One last note, I think the integration tests I added cover all the branches.

jasontedor added some commits Feb 20, 2019

@martijnvg

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@jasontedor That should work too; the ilm unfollow step can check if the es.failed_to_remove_retention_leases metadata has been set and not threat that as a regular failure.

@martijnvg
Copy link
Member

left a comment

LGTM

@jasontedor jasontedor merged commit ae7b946 into elastic:master Feb 20, 2019

8 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 20, 2019

Remove retention leases when unfollowing (elastic#39088)
This commit attempts to remove the retention leases on the leader shards
when unfollowing an index. This is best effort, since the leader might
not be available.

jasontedor added a commit that referenced this pull request Feb 20, 2019

Remove retention leases when unfollowing (#39088)
This commit attempts to remove the retention leases on the leader shards
when unfollowing an index. This is best effort, since the leader might
not be available.

jasontedor added a commit that referenced this pull request Feb 20, 2019

Remove retention leases when unfollowing (#39088)
This commit attempts to remove the retention leases on the leader shards
when unfollowing an index. This is best effort, since the leader might
not be available.

@jasontedor jasontedor deleted the jasontedor:retention-lease-unfollow branch Feb 20, 2019

jasontedor added a commit that referenced this pull request Feb 20, 2019

Remove retention leases when unfollowing (#39088)
This commit attempts to remove the retention leases on the leader shards
when unfollowing an index. This is best effort, since the leader might
not be available.

@jakelandis jakelandis added v7.0.0-rc2 and removed v7.0.0 labels Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.