Clean up TransportResizeAction
#137471
Clean up TransportResizeAction
#137471
Conversation
DaveCTurner
added
>non-issue
:Data Management/Indices APIs
Mainly this is about removing the trappy implicit default master node timeout in resize (shrink/split/clone) requests as per elastic#107984, but also we drop the unnecessary `ActionType` subclass and the test-only `ResizeRequestBuilder`, migrating the affected tests over to a new `ResizeIndexTestUtils` instead. We also fix the order of index-name arguments and make the `ResizeType` a mandatory parameter rather than (trappily) defaulting to `SHRINK`.
DaveCTurner
force-pushed
the
2025/10/31/resize-action-cleanup
branch
from
4c28f55 to
a6974e5
Compare
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
nielsbauman
left a comment
nielsbauman
left a comment
There was a problem hiding this comment.
I'm a fan of these changes, thanks a lot, David! I've left a few comments, but they're only nits.
| public enum ResizeIndexTestUtils { | ||
| ; |
There was a problem hiding this comment.
Is there value in having this be an enum instead of a class (optionally with a private empty constructor)?
There was a problem hiding this comment.
Don't know really - we seem to do both and this is fewer LoC since you don't have to add the private empty constructor.
...ms/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java
Show resolved
Hide resolved
...rc/internalClusterTest/java/org/elasticsearch/action/admin/indices/create/ShrinkIndexIT.java
Outdated
Show resolved
Hide resolved
...rc/internalClusterTest/java/org/elasticsearch/action/admin/indices/create/ShrinkIndexIT.java
Show resolved
Hide resolved
...erTest/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| private CreateIndexRequest targetIndexRequest; |
There was a problem hiding this comment.
Did you consider making this field final as well? It's only reassigned by ResizeRequest#setTargetIndex, which, as of this PR, is only used two times in ResizeRequestTests. This isn't a blocker for me, I'm fine leaving it as-is.
There was a problem hiding this comment.
I did but tbh these tests are kinda questionable, and I didn't really want to rework them enough to avoid mutating this field. It's not a huge deal, really it's a bigger problem that the fields within CreateIndexRequest are themselves mutable, but that's a problem for another day.
server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequestTests.java
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeActionTests.java
Show resolved
Hide resolved
...lugin/logsdb/src/internalClusterTest/java/org/elasticsearch/xpack/logsdb/LogsIndexingIT.java
Show resolved
Hide resolved
...t/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptorTests.java
Outdated
Show resolved
Hide resolved
3 participants
Mainly this is about removing the trappy implicit default master node
timeout in resize (shrink/split/clone) requests as per #107984, but also
we drop the unnecessary
ActionTypesubclass and the test-onlyResizeRequestBuilder, migrating the affected tests over to a newResizeIndexTestUtilsinstead. We also fix the order of index-namearguments and make the
ResizeTypea mandatory parameter rather than(trappily) defaulting to
SHRINK.