Finalize acknowledgements work, move over all the api, strenghten ack api and remove boiler plate code #4051
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uced generic ack mechanism
…duced generic ack mechanism
…d generic ack mechanism Side note: the double notification from each node (one after deleting from metadata, one after deleting from file system) that was in place is not needed anymore, as each node returns the ack in the clusterStateProcessed, which gets called after all the listeners (one of which actually deletes from file system)
… generic ack mechanism
…d generic ack mechanism
…ngs, put warmer and delete warmer Helps consistency between the different apis that modify the cluster state and support acknowledgements
Introduced TransportClusterStateUpdateAction base class that extends TransportMasterNodeOperationAction to remove boiler plate code from all subclasses. Introduced abstract ClusterStateUpdateActionListener class that implements ClusterStateUpdateListener. Made AckedClusterStateUpdateTask an abstract class and added its default subcass that is used in most of the cases (AckedDefaultStateUpdate) unless a specific ClusterStateUpdateResponse subclass (that holds more than the only acknowledged flag) is needed. This allowed to remove boiler plate code from all MetaDataServices (onResponse, onTimeout, ackTimeout, masterNodeTimeout, onFailure and so on) Forced also implementing toString on all ClusterStateUpdateRequest, used for logging (debug level on the action package) in case something goes wrong updating the cluster state. Previously only some of the apis did this. The only api that hasn't been fully migrated yet is the delete index api (TransportDeleteIndexAction doesn't subclass yet TransportClusterStateUpdateAction) as it supports multiple indices but submits a different cluster state update per index. We might wanto to keep it like this for now as it also locks the index during the delete operation.
… action.support.master package
Added common method that read the timeout and master_timeout parameter from the rest request and sets them to the AcknowledgedRequest Made sure that the rest layer doesn't have its own default values for timeout and master_timeout (used the ones already set in the request class) Moved Put Alias and Delete Alias over to the new AcknowledgedRestListener Made ok and acknowledged XContentString constants Fixed unchecked cast in rest put/delete warmer
As a side note both api read the timeout parameter and return the acknowledged flag, although they don't support acknowledgements. The timeout parameter is ignored and the acknowledged flag is always true. Kept that behaviour for now. The difference with the other api is the type of ClusterStateUpdateTask submitted, which is TimeoutClusterStateUpdateTask instead of AckedClusterStateUpdateTask. The rest of the code could be migrated anyway to the new structure.
ghost
assigned 
|
Closing this one. Going to send smaller pull requests per api instead of this big one that's hard to review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All the apis that modify the cluster state and already supported acknowledgement but used the old mechanism with custom notifications have been migrated to the new generic ack mechanism introduced in #3786 : indices aliases, open/close index, delete index, put mapping, create index.
A MetaDataService that actually submits the cluster state update task has been introduced in cluster reroute, cluster update settings, put/delete warmer (similar to what other apis already do) rather than making changes directly in the
Transport*Action. TheAcknowledgedRequestgets converted to an internalClusterStateUpdateRequest(or subclass) that is used for the cluster state update which returns aClusterStateUpdateResponse(or subclass), that needs to be converted back toAcknowledgedResponse.Introduced a
TransportClusterStateUpdateActionbase class that allows to share code and avoid repeating boiler plate code in each action. MadeAckedClusterStateUpdateTaskan abstract class that contains the implementation for the methods that are always the same all over the place (similar to #4001). Unified the rest layer too to parse thetimeoutandmaster_timeoutparameters in a single place.The
TransportDeleteIndexActionis the only one that has not been moved over toTransportClusterStateUpdateActionas it needs some rework: it currently performs multiple cluster state updates, one per index, and there's a metadata lock involved per each index.Also, I wonder if we should go ahead and add ack support in put/delete index template as they both already support all the needed parameters, which are currently pointless (timeout ignored, acknowledged always true).