Skip to content

Reindex share retry between hit sources #44203

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

Merged

Conversation

henningandersen
Copy link
Contributor

The client and remote hit sources had each their own retry mechanism,
which would do the same. Supporting resiliency we would have to expand
on the retry mechanisms and as a preparation for that, the retry
mechanism is now shared such that each sub class is only responsible for
sending requests and converting responses/failures to common format.

Part of #42612

The client and remote hit sources had each their own retry mechanism,
which would do the same. Supporting resiliency we would have to expand
on the retry mechanisms and as a preparation for that, the retry
mechanism is now shared such that each sub class is only responsible for
sending requests and converting responses/failures to common format.

Part of elastic#42612
@henningandersen henningandersen added >non-issue v8.0.0 :Distributed Indexing/Reindex Issues relating to reindex that are not caused by issues further down v7.4.0 labels Jul 11, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@henningandersen
Copy link
Contributor Author

@elasticmachine update-branch

@henningandersen
Copy link
Contributor Author

@elasticmachine update branch

@henningandersen
Copy link
Contributor Author

Failure looks idential to #43889 but for another test,
@elasticmachine run elasticsearch-ci/2

Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

LGTM - I added a comment related to my preferences. But if you feel strongly otherwise, I'm fine with it.

/**
* Return a new listener that delegates failure/reject to original but forwards response to responseHandler
*/
default <X> RejectAwareActionListener<X> withResponseHandler(Consumer<X> responseHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is for default to be used in a scenario where you are providing default behavior for an interface that some implementation might want to overwrite. This feels more like a helper static method such as:

static <X> RejectAwareActionListener<X> withResponseHandler(RejectAwareActionListener<X> errorDelegate, Consumer<X> responseHandler) {
// Implementation
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Fixed in 07d4793

No intention to override witResponsseHandler method so static is more
appropriate.
…rsen/elasticsearch into enhance_reindex_shared_retry
@henningandersen henningandersen merged commit c8977a4 into elastic:master Aug 6, 2019
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Aug 8, 2019
The client and remote hit sources had each their own retry mechanism,
which would do the same. Supporting resiliency we would have to expand
on the retry mechanisms and as a preparation for that, the retry
mechanism is now shared such that each sub class is only responsible for
sending requests and converting responses/failures to common format.

Part of elastic#42612
henningandersen added a commit that referenced this pull request Aug 8, 2019
The client and remote hit sources had each their own retry mechanism,
which would do the same. Supporting resiliency we would have to expand
on the retry mechanisms and as a preparation for that, the retry
mechanism is now shared such that each sub class is only responsible for
sending requests and converting responses/failures to common format.

Part of #42612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Reindex Issues relating to reindex that are not caused by issues further down >non-issue v7.4.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants