Pull index routing into strategy object#77211
Pull index routing into strategy object#77211elasticsearchmachine merged 6 commits intoelastic:masterfrom
Conversation
This pulls the calculation of the shard id for an (id, routing) pair into a little strategy class, `IndexRouting`. This is easier to test and should be easier to extend. My hope is that this is an incremental readability improvement. My ulterior motive is that this is where I want to land our new routing-by-dimensions work for tsdb.
|
Pinging @elastic/es-distributed (Team:Distributed) |
nik9000
left a comment
There was a problem hiding this comment.
I feel bad adding another per index object, but ultimately tsdb is going to need a larger object in the same place. In my mind its just a different subclass of IndexRouting. And it'll contain a compiled matcher-like-thing. So I think I want to build some way to deduplicate these things sooner rather than later.
| } | ||
| } | ||
|
|
||
| public void testPartitionedIndex() { |
There was a problem hiding this comment.
This and all tests below it duplication OperationRoutingTests. They are slightly lower level in that they don't test the creation of the IndexRouting from information in the IndexMetadata. But they are close. Paranoia drove me to duplicate them rather than move them.
There was a problem hiding this comment.
Now that I've reworked how this is integrated I've moved these test from OperationRoutingTests .
|
We discussed this on another channel and Nik will work on a different solution, trying to just share the |
nik9000
left a comment
There was a problem hiding this comment.
@henningandersen I've updated this based on our conversation this morning.
| } | ||
| } | ||
|
|
||
| public void testPartitionedIndex() { |
There was a problem hiding this comment.
Now that I've reworked how this is integrated I've moved these test from OperationRoutingTests .
|
run elasticsearch-ci/part-2 |
henningandersen
left a comment
There was a problem hiding this comment.
LGTM, thanks Nik.
| IndexRouting indexRouting = indexRoutings.computeIfAbsent( | ||
| concreteIndex, | ||
| idx -> IndexRouting.fromIndexMetadata(clusterState.metadata().getIndexSafe(idx)) | ||
| ); | ||
| ShardId shardId = clusterService.operationRouting() | ||
| .indexShards(clusterState, concreteIndex.getName(), indexRouting, docWriteRequest.id(), docWriteRequest.routing()) | ||
| .shardId(); |
There was a problem hiding this comment.
As a future refinement (not necessarily in this PR), I wonder if OperationRouting should create the IndexRouting and the IndexRouting object then should have the indexShards (and more) method(s)? So the interaction here would be something like:
| IndexRouting indexRouting = indexRoutings.computeIfAbsent( | |
| concreteIndex, | |
| idx -> IndexRouting.fromIndexMetadata(clusterState.metadata().getIndexSafe(idx)) | |
| ); | |
| ShardId shardId = clusterService.operationRouting() | |
| .indexShards(clusterState, concreteIndex.getName(), indexRouting, docWriteRequest.id(), docWriteRequest.routing()) | |
| .shardId(); | |
| ShardId shardId = indexRoutings.computeIfAbsent( | |
| concreteIndex, | |
| idx -> clusterService.operationRouting().indexRouting(clusterState, idx)) | |
| ).indexShards(docWriteRequest.id(), docWriteRequest.routing()) | |
| .shardId(); |
I do see the problem in doing so for ShardSplittingQuery though. And it may be affected by your future work too, where you may want bulk to only create a new IndexRouting instance when the base parameters are different. So I am OK leaving this as is for now if you prefer to not tackle that now.
There was a problem hiding this comment.
Moving the methods like indexShards into makes sense. I liked that the class as it stands now doesn't need to know about stuff like the routing table. I think which was is right will show up more clearly after I add the source based routing stuff so I'll keep it as it is for now. Once I have a proposal for source based routing.
| * Build the routing from {@link IndexMetadata}. | ||
| */ | ||
| public static IndexRouting fromIndexMetadata(IndexMetadata indexMetadata) { | ||
| if (indexMetadata.getRoutingPartitionSize() == 1) { |
There was a problem hiding this comment.
nit, why not use the isRoutingPartitionedIndex:
| if (indexMetadata.getRoutingPartitionSize() == 1) { | |
| if (indexMetadata.isRoutingPartitionedIndex() == false) { |
There was a problem hiding this comment.
I went back and forth. I suppose I had the effects of the partition size in my head and liked the way I had it better. But I think you are right its more clear that way. I'll push the change.
💔 Backport failed
To backport manually run |
This pulls the calculation of the shard id for an (id, routing) pair into a little strategy class, `IndexRouting`. This is easier to test and should be easier to extend. My hope is that this is an incremental readability improvement. My ulterior motive is that this is where I want to land our new routing-by-dimensions work for tsdb.
This pulls the calculation of the shard id for an (id, routing) pair into a little strategy class, `IndexRouting`. This is easier to test and should be easier to extend. My hope is that this is an incremental readability improvement. My ulterior motive is that this is where I want to land our new routing-by-dimensions work for tsdb.
This pulls the calculation of the shard id for an (id, routing) pair
into a little strategy class,
IndexRouting. This is easier to test andshould be easier to extend.
My hope is that this is an incremental readability improvement. My
ulterior motive is that this is where I want to land our new
routing-by-dimensions work for tsdb.