-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[ML] Disable CPS for Dataframes #136716
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] Disable CPS for Dataframes #136716
Conversation
Cross-Project Search and Cross-Cluster Search is indefinitely disabled for Dataframe Analytics. The error message will now display if the syntax would have otherwise resolved to the respective feature.
|
Pinging @elastic/ml-core (Team:ML) |
| context.addValidationError( | ||
| context.crossProjectEnabled() ? CROSS_PROJECT_INDICES_NOT_SUPPORTED : REMOTE_SOURCE_INDICES_NOT_SUPPORTED | ||
| ); |
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 is the main bit of changed logic - if we're in an environment that supports cross-project, then we are also in an environment that does not support cross-cluster, so we display the appropriate error message
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.
@prwhelan do we really need to have two separate validation failures? Why not just one that says "remote source and cross-project indices are not supported". Seems like an unnecessary complication.
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'm fine with that, though this code will come back for the transform check
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.
@prwhelan right, transforms only support remote indices, not cps. I think thats fine.
| transportService.getRemoteClusterService(), | ||
| null, | ||
| null, | ||
| new CrossProjectModeDecider(clusterService.getSettings()).crossProjectEnabled(), |
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 initially tried injecting either the CrossProjectModeDecider and just a boolean, but the powers that be don't like it when both Transform and MachineLearning declare the same objects as a component, and I couldn't figure out a way around that, so we're just parsing the boolean here and sending it down for now. We just need the boolean anyway, and it is a static setting so it won't change unless the cluster reboots anyway.
|
@benwtrent what's your take on this failing REST test? i changed the file, but it looks like it's checking out 8.19 and running that test - should i be using a NodeFeature to verify that the cluster is at the current version, else return the old exception message? |
|
@prwhelan exact error messages aren't really part of our API contracts. We should really only be asserting on if its a I would suggest skipping the compat test that is failing. We should adjust the assertions in 8.19 in a separate PR. Look for an example on how to do this skipping in gradle files like: |
Cross-Project Search and Cross-Cluster Search is indefinitely disabled for Dataframe Analytics. The error message will now display if the syntax would have otherwise resolved to the respective feature.
Cross-Project Search and Cross-Cluster Search is indefinitely disabled for Dataframe Analytics. The error message will now display if the syntax would have otherwise resolved to the respective feature.
Cross-Project Search and Cross-Cluster Search is indefinitely disabled for Dataframe Analytics. The error message will now display if the syntax would have otherwise resolved to the respective feature.