-
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
[FEATURE][ML] Stregthen source dest validations for DF analytics #43399
[FEATURE][ML] Stregthen source dest validations for DF analytics #43399
Conversation
This adds the following validations: - dest index name is valid - source index exists - dest index is not included in source index - dest index is matching a single index at most
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.
✅
this.indexNameExpressionResolver = Objects.requireNonNull(indexNameExpressionResolver); | ||
} | ||
|
||
public void check(DataFrameAnalyticsConfig config) { |
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 suppose it is just a Java thing, but I have trouble with creating an object with state, but an internal method is only called once. Seems like a static check(ClusterState clusterState, IndexNameExpressionResolver indexNameExpressionResolver, DataFrameAnalyticsConfig config)
is fewer lines of code and less state to break.
Of course, I am hypocritical with this :). I often make these classes myself.
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 know what you mean. I insist going for objects though because I think they lend themselves better for expansion. On the other hand, when one sees a static method it discourages refactoring to make an object. Of course, only time will tell. My view is that when paradigms are not clearly better or worse than alternatives, we have to try them out and wait for empirical evidence to reward us or slap us in the face :-)
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
I just left one idea for another test
"index": "index-source" | ||
}, | ||
"dest": { | ||
"index": "<script>Foo" |
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.
It might be worth adding one more test, where the destination index name is a valid wildcard, e.g. mydest*
, to prove that wildcarded destinations are not allowed.
This adds the following validations to the put and start data frame analytics APIs: