Changed every single index operation to not replace the index within the original request #7223
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
| this.metaData = metaData; | ||
| } | ||
|
|
||
| String put(String index, IndicesOptions indicesOptions) { |
There was a problem hiding this comment.
This class confuses me a bit since it is not really a hash table and this method is not really a put. Maybe not extend HashMap and have more explicit method name?
There was a problem hiding this comment.
(but it could wrap a hashmap)
There was a problem hiding this comment.
sure, I agree, will change
|
I left some questions, but big +1 on the proposed refactoring! |
|
Updated according to feedback I got |
|
LGTM |
…ex within the original request An anti-pattern that we have in our code, noticeable for java API users, is that we modify incoming requests by replacing the index or alias with the concrete index. This way not only the request has changed, but all following communications that use that request will lose the information on whether the original request was performed against an alias or an index. Refactored the following base classes: `TransportShardReplicationOperationAction`, `TransportShardSingleOperationAction`, `TransportSingleCustomOperationAction`, `TransportInstanceSingleOperationAction` and all subclasses by introduced an InternalRequest object that contains the original request plus additional info (e.g. the concrete index). This internal request doesn't get sent over the transport but rebuilt on each node on demand (not different to what currently happens anyway, as concrete index gets set on each node). When the request becomes a shard level request, instead of using the only int shardId we serialize the ShardId that contains both concrete index name (which might then differ ffrom the original one within the request) and shard id. Using this pattern we can move get, multi_get, explain, analyze, term_vector, multi_term_vector, index, delete, update, bulk to not replace the index name with the concrete one within the request. The index name within the original request will stay the same. Made it also clearer within the different transport actions when the index needs to be resolved and when that's not needed (e.g. shard level request), by exposing `resolveIndex` method. Moved check block methods to parent classes as their content was always the same on every subclass. Improved existing tests by randomly introducing the use of an alias, and verifying that the responses always contain the concrete index name and not the original one, as that's the expected behaviour. Added backwards compatibility tests to make sure that the change is applied in a backwards compatible manner. Closes elastic#7223
javanna
added a commit
that referenced
this pull request
Aug 14, 2014
…hardSingleOperationAction relates to #7223
javanna
added a commit
that referenced
this pull request
Aug 14, 2014
…hardSingleOperationAction relates to #7223
javanna
added a commit
that referenced
this pull request
Sep 8, 2014
…ex within the original request An anti-pattern that we have in our code, noticeable for java API users, is that we modify incoming requests by replacing the index or alias with the concrete index. This way not only the request has changed, but all following communications that use that request will lose the information on whether the original request was performed against an alias or an index. Refactored the following base classes: `TransportShardReplicationOperationAction`, `TransportShardSingleOperationAction`, `TransportSingleCustomOperationAction`, `TransportInstanceSingleOperationAction` and all subclasses by introduced an InternalRequest object that contains the original request plus additional info (e.g. the concrete index). This internal request doesn't get sent over the transport but rebuilt on each node on demand (not different to what currently happens anyway, as concrete index gets set on each node). When the request becomes a shard level request, instead of using the only int shardId we serialize the ShardId that contains both concrete index name (which might then differ ffrom the original one within the request) and shard id. Using this pattern we can move get, multi_get, explain, analyze, term_vector, multi_term_vector, index, delete, update, bulk to not replace the index name with the concrete one within the request. The index name within the original request will stay the same. Made it also clearer within the different transport actions when the index needs to be resolved and when that's not needed (e.g. shard level request), by exposing `resolveIndex` method. Moved check block methods to parent classes as their content was always the same on every subclass. Improved existing tests by randomly introducing the use of an alias, and verifying that the responses always contain the concrete index name and not the original one, as that's the expected behaviour. Added backwards compatibility tests to make sure that the change is applied in a backwards compatible manner. Closes #7223
javanna
added a commit
that referenced
this pull request
Sep 8, 2014
…hardSingleOperationAction relates to #7223
clintongormley
changed the title
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.
An anti-pattern that we have in our code, noticeable if you use java API, is that we modify incoming requests by replacing the index or alias with the concrete index. This way not only the original user request has changed, but all following communications that use that request will lose the information on whether the original request was performed against an alias or an index, and its name.
Refactored the following base classes:
TransportShardReplicationOperationAction,TransportShardSingleOperationAction,TransportSingleCustomOperationAction,TransportInstanceSingleOperationActionand all subclasses by introducing an InternalRequest object that holds the original request plus additional info (e.g. the concrete index). This internal request doesn't get sent over the transport but rebuilt on each node on demand (not different to what currently happens anyway, as concrete index gets re-set on each node). When the request becomes a shard level request, instead of using the only int shardId we serialize theShardIdthat contains both concrete index name (which might then differ from the original one within the request) and shard id.Using this pattern we can move get, multi_get, explain, analyze, term_vector, multi_term_vector, index, delete, update, bulk to not replace the index name with the concrete one within the request. The index name within the original request will stay the same.
Made it also clearer within the different transport actions when the index needs to be resolved (user facing requests) and when that's not needed (e.g. shard level request), by exposing
resolveIndexmethod. Moved check block methods to parent classes as their content was always the same on every subclass.Improved existing tests by randomly introducing the use of an alias, and verifying that the responses always contain the concrete index name and not the original one, as that's the expected behaviour.
Added backwards compatibility tests to make sure that the change is applied in a backwards compatible manner.