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

Refactored open/close index api to make use of the new recently introduced generic ack mechanism #4170

Merged
merged 3 commits into from Nov 20, 2013

Conversation

javanna
Copy link
Member

@javanna javanna commented Nov 14, 2013

Refactored open/close index api to make use of the new recently introduced generic ack mechanism

Closes #4169

@ghost ghost assigned javanna Nov 15, 2013
/**
* Cluster state update request that allows to close one or more indices
*/
public class CloseIndexClusterStateUpdateRequest extends ClusterStateUpdateRequest<CloseIndexClusterStateUpdateRequest> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class looks exactly the same as the OpenIndexClusterStateUpdateRequest, maybe we can simplify this and factor it out. Seems like it's common?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep we can do that. The only reason why I didn't do it was because usually these classes that extend ClusterStateUpdateRequest can only be created in the same package, and open and close are two different packages... thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having the classes is fine they can just be marker classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense

@s1monw
Copy link
Contributor

s1monw commented Nov 19, 2013

this PR looks good to me but I still need to understand why NodeIndicesStateUpdatedAction is now gone entirely.
This might not be ported to 0.90.x since it removes transport classes right?

@javanna
Copy link
Member Author

javanna commented Nov 19, 2013

Thanks for the review! The change is similar to what I did in #4115 for aliases. We can backport it as well as explained there. Those custom notifications sent back to the master may be sent to a non existing endpoint during a rolling upgrade, and that wouldn't cause any trouble as exception sent back from the master are literally ignored in that case (see EmptyTransportResponseHandler).

@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2013

The updates LGTM +1 to push

…ed methods variation without version as parameter, to be used by api that always read and write those parameters. Helps avoiding to call the variation that accepts a Version with actual version null.
@javanna javanna merged commit adb8318 into elastic:master Nov 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move open/close index api to new acknowledgement mechanism
2 participants