Adds REST client support for starting and stopping ILM #32609
Conversation
colings86
added
the
:Data Management/ILM+SLM
|
Pinging @elastic/es-core-infra |
|
Any thoughts on my suggestions above? |
|
I prefer (1.) I re-used the same transport request for simplicity under the hood, I do not think this consolidation should be exposed to the user, and users should see specific objects that reflect what they can or cannot do. |
dakrone
left a comment
dakrone
left a comment
There was a problem hiding this comment.
Not sure if this is the best way to expose this API. I think given the REST api for this API are separate endpoints (_ilm/start and _ilm/stop) that we should either:
- Split this API into two on the transport layer as well and have a StartILMAction and a StopILMAction with their own request and response objects
- Keep the action as is but hide the PutOperationRequest from the user and have the client expose a PutOperationModeResponse startILM(RequestiOptions) method and a PutOperationModeResponse stopILM(RequestOptions) method.
I think I'm more in favor of option 1, even though it will require slightly more code.
I think this helps us not have to expose the PutOperationModeRequest and the Enum that (I agree) is not particularly friendly to a user. So whatever we can do to avoid exposing that would be great.
There was a problem hiding this comment.
Think it'd be worth renaming this to putLifecycleOperationMode? I'm worried the name is too generic for a gigantic file that isn't specific to ILM
colings86
changed the title
dakrone
left a comment
dakrone
left a comment
There was a problem hiding this comment.
I left some comments, could you remove the new Response classes and use AcknowledgedResponse everywhere please
There was a problem hiding this comment.
Indentation is off here and the line below (super minor)
There was a problem hiding this comment.
How did we arrive at this? (not critiquing, just curious)
There was a problem hiding this comment.
random number so no method to pick it really. The problem is that we need to be able to do equality so it has to have some value. Ideally I would have an INSTANCE constant and make the constructor private but the Streamable serialisation relies on the constructor being public
There was a problem hiding this comment.
Can you remove this class entirely in favor of just returning AcknowledgedResponse? (similar to #32722)
There was a problem hiding this comment.
Can you also remove this one in favor of AcknowledgedRequest?
There was a problem hiding this comment.
Whoops sorry, ignore this, should only change the Response ones
|
@dakrone I pushed a commit that should address your comments |
* Adds REST client support for PutOperationMode in ILM * Corrects licence headers * iter * add request converter test * Fixes tests * Creates start and stop actions for controlling ILM operation * Addresses review comments
4 participants
Not sure if this is the best way to expose this API. I think given the REST api for this API are separate endpoints (
_ilm/startand_ilm/stop) that we should either:StartILMActionand aStopILMActionwith their own request and response objectsPutOperationRequestfrom the user and have the client expose aPutOperationModeResponse startILM(RequestiOptions)method and aPutOperationModeResponse stopILM(RequestOptions)method.In either case I'm not sure its a good idea to expose the current
PutOperationModeRequestclass to the user since its not particularly user friendly because:a. As far as the user is concerned what they want to do is start and stop ILM not put it into an "operation mode"
b. The
OperationModeenum itself is not particular user friendly since its values areRUNNING,STOPPINGandSTOPPEDwhich relate to the state ILM is in not what you want to do as a user and also becausePutOperationModeRequestrejectsSTOPPEDbecause you can't go fromRUNNINGstraight toSTOPPEDand this is an implementation detail the user should not need to be concerned with when they use this APIOpen to thoughts.