-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML-Dataframe] add stop and delete endpoints #33597
[ML-Dataframe] add stop and delete endpoints #33597
Conversation
Pinging @elastic/ml-core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good apart from a few minor nits
public TransportDeleteFeatureIndexBuilderJobAction(Settings settings, TransportService transportService, ThreadPool threadPool, | ||
ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver, | ||
PersistentTasksService persistentTasksService, ClusterService clusterService) { | ||
super(settings, DeleteFeatureIndexBuilderJobAction.NAME, transportService, clusterService, threadPool, actionFilters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has changed on master. threadpool
and indexNameExpressionResolver
are no longer passed to super
. When you are in between PRs it's probably a good idea to merge master into your feature branch and resolve these differences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 will merge master and repair the code after merging this PR
this.state = state; | ||
this.currentPosition = position == null ? null : new TreeMap<>(position); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intention is for this to be immutable then you should wrap the new TreeMap
with Collections.unmodifiableSortedMap()
, because otherwise a caller can modify it via the getter.
} | ||
|
||
public FeatureIndexBuilderJobState(StreamInput in) throws IOException { | ||
state = IndexerState.fromStream(in); | ||
currentPosition = in.readBoolean() ? new TreeMap<>(in.readMap()) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above, new TreeMap
should be wrapped with Collections.unmodifiableSortedMap()
.
import org.elasticsearch.xpack.ml.featureindexbuilder.action.StopFeatureIndexBuilderJobAction.Request; | ||
import java.io.IOException; | ||
|
||
public class StopFeatureIndexBuilderJobActionRequestTests extends AbstractXContentTestCase<Request> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should extend AbstractStreamableTestCase
instead of AbstractXContentTestCase
. As it stands at the moment this test is not testing that the wire streaming read/write methods are consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
FEATURE BRANCH PR
add rest endpoints to stop and delete feature build indexer tasks and fix task saving/reloading