[Transform] finalize feature reset integration #71133
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
benwtrent
force-pushed
the
feature/transform-feature-reset-integration
branch
from
546646d to
75ad1c5
Compare
…-feature-reset-integration
…-feature-reset-integration
| @@ -213,6 +220,7 @@ protected XPackLicenseState getLicenseState() { | |||
| new ActionHandler<>(GetTransformStatsAction.INSTANCE, TransportGetTransformStatsAction.class), | |||
| new ActionHandler<>(PreviewTransformAction.INSTANCE, TransportPreviewTransformAction.class), | |||
| new ActionHandler<>(UpdateTransformAction.INSTANCE, TransportUpdateTransformAction.class), | |||
| new ActionHandler<>(SetResetModeAction.INSTANCE, TransportSetResetModeAction.class), | |||
There was a problem hiding this comment.
makes me wonder: all the other actions have Transform in the name, this one not
There was a problem hiding this comment.
No particular reason. I can rename it
| .setActions(TransformField.TASK_NAME) | ||
| .setWaitForCompletion(true) | ||
| .execute(ActionListener.wrap( | ||
| listMlTasks -> { |
There was a problem hiding this comment.
nit: listMlTasks -> listTransformTasks
|
|
||
| StopTransformAction.Request stopTransformsRequest = new StopTransformAction.Request(Metadata.ALL, true, true, null, true, false); | ||
| client.execute(StopTransformAction.INSTANCE, stopTransformsRequest, afterStoppingTransforms); | ||
| client.execute(SetResetModeAction.INSTANCE, SetResetModeActionRequest.enabled(), afterResetModeSet); |
There was a problem hiding this comment.
what happens if something goes wrong between set and reset? E.g. node crashes between the 2. Will reset mode stay in cluster state in this case? What is the mitigation? Do I have to call reset again?
There was a problem hiding this comment.
Will reset mode stay in cluster state in this case?
Yes
What is the mitigation? Do I have to call reset again?
Yes
| } | ||
|
|
||
| /** | ||
| * Merge the diff with the ML metadata. |
| ClusterState state, | ||
| ActionListener<AcknowledgedResponse> listener) throws Exception { | ||
|
|
||
| final boolean isResetModelEnabled = isResetMode(state); |
There was a problem hiding this comment.
an l too much: isResetModelEnabled -> isResetModeEnabled
| ActionListener<AcknowledgedResponse> clusterStateUpdateListener = ActionListener.wrap( | ||
| acknowledgedResponse -> { | ||
| if (acknowledgedResponse.isAcknowledged() == false) { | ||
| logger.info(new ParameterizedMessage("Cluster state update is NOT acknowledged for [{}]", featureName())); |
There was a problem hiding this comment.
^ looks like a debug leftover to me
| success -> client.execute(SetResetModeAction.INSTANCE, SetResetModeActionRequest.disabled(), ActionListener.wrap( | ||
| resetSuccess -> finalListener.onResponse(success), | ||
| resetFailure -> { | ||
| logger.error("failed to disable reset mode after otherwise successful transform reset", resetFailure); |
There was a problem hiding this comment.
it would be good to make this actionable. If I see this error as a user I don't know what I should do, should I call reset again?
| resetFailure -> { | ||
| logger.error("failed to disable reset mode after otherwise successful transform reset", resetFailure); | ||
| finalListener.onFailure( | ||
| ExceptionsHelper.serverError( |
There was a problem hiding this comment.
I am not sure about this one. It lacks a proper status code. If I get it right, it does not set one, which means it is based on the one from resetFailure, which could be anything. I think a status exception with a proper code would be better. My gut feeling would be a 503 - but that depends on the failure. Related to my comment above: what should a user do? A 503 would indicate a retry.
There was a problem hiding this comment.
serverError should throw an internal server error. If it doesn't I am misusing it and should throw an internal server error.
| .setActions("indices:data/write/bulk") | ||
| .setDetailed(true) | ||
| .setWaitForCompletion(true) | ||
| .setDescriptions("*.transform-*", "*.data-frame-*") |
There was a problem hiding this comment.
consider using contants from TransformInternalIndexConstants
…-feature-reset-integration
…-feature-reset-integration
This commit updates transform feature reset to: - wait for transform tasks to complete - wait for all indexing actions to transform indices to complete - and prevents transform audit messages from being written while the reset is being processed related to elastic#70008 & elastic#69581
* [Transform] finalize feature reset integration (#71133) This commit updates transform feature reset to: - wait for transform tasks to complete - wait for all indexing actions to transform indices to complete - and prevents transform audit messages from being written while the reset is being processed related to #70008 & #69581
This commit updates transform feature reset to:
related to #70008 & #69581