Don't replace indices within ActionRequest and check blocks against concrete indices #6777
Conversation
…rationAction#masterOperation method If the request is being executed on the master node, there is no need to replace its content with the concrete indices obtained by calling MetaData#concreteIndices. Leave the request as it is and use the concrete indices to perform the requested operation.
…crete ones, and make sure check blocks is executed on concrete indices Concrete indices is now called multiple times when needed instead of changing what's inside the incoming request with the concrete indices. Ideally we want to keep the original aliases or indices or wildcard expressions in the request. Also made sure that the check blocks is done against the concrete indices, which wasn't the case for delete index, delete mapping, open index, close index, types exists and indices exists. Closes elastic#6694 Closes elastic#6777
| protected ClusterBlockException checkBlock(IndicesExistsRequest request, ClusterState state) { | ||
| return state.blocks().indicesBlockedException(ClusterBlockLevel.METADATA, request.indices()); |
There was a problem hiding this comment.
we can potentially have a concreteIndices(ClusterService service, IndicesOptions options) on the request itself and have an internal transient field to hold the concrete indices that is set lazily. So you won't need to compute the concrete indices more than once?
There was a problem hiding this comment.
We could do that, I'm just a bit hesitant on having to copy paste this new field to many requests as we don't have a single base class that deals with indices.
|
I think we should also change |
|
Regarding the indices field, I would love to not expose being able to set it, its much cleaner. I would not expose a setter for it, not in the request layer and not in the builder layer, and only allow to create it as a constructor parameter. That is a much bigger change, so maybe for starters we can do this one, and then do the other one. |
|
@uboness regarding the |
|
@kimchy agreed, that would require changes to request builders and/or action hierarchy though, we'll see what we can do on a separate issue. |
|
LGTM |
|
LGTM |
…e ones, and make sure check blocks is executed on concrete indices Concrete indices is now called multiple times when needed instead of changing what's inside the incoming request with the concrete indices. Ideally we want to keep the original aliases or indices or wildcard expressions in the request. Also made sure that the check blocks is done against the concrete indices, which wasn't the case for delete index, delete mapping, open index, close index, types exists and indices exists. Closes #6694 Closes #6777
clintongormley
changed the title
clintongormley
changed the title
If the request is being executed on the master node, there is no need to replace its content with the concrete indices obtained by calling MetaData#concreteIndices. Leave the request as it is and use the concrete indices to perform the requested operation (commit #1).
Concrete indices is now called multiple times when needed instead of changing what's inside the incoming request with the concrete indices. Ideally we want to keep the original aliases or indices or wildcard expressions in the request (commit #2).
Also made sure that the check blocks is done against the concrete indices, which wasn't the case for delete index, delete mapping, open index, close index, types exists and indices exists (commit #2).
Note: I think replacing the indices within the request is a bad pattern and I would love to find a way to make the indices final somehow, so that we don't make the same mistake again in the future. On the other hand, requests are exposed through java api and serialized over the wire, which both make it hard to make the indices field final and to remove the setter, which needs to be exposed to the request builders. I thought about making the setter package private, but that wouldn't help as the transport action is usually in the same package and the same could happen again. Ideas are welcome here!