ES|QL: Move SET command out of snapshot and move validation to single settings #137204
Conversation
elasticsearchmachine
added
v9.3.0
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
ivancea
left a comment
ivancea
left a comment
There was a problem hiding this comment.
Looks good, thanks!
With some luck, this PR adding SET tests is merged soon too, so the SET statement is "fully scaffolded"
| import org.elasticsearch.Build; | ||
| import org.elasticsearch.transport.RemoteClusterService; | ||
|
|
||
| public class SettingsValidationContext { |
There was a problem hiding this comment.
nit: As the tests are subclassing this, and to avoid calling Build.current().isSnapshot() multiple times in production, maybe this should be a record (crossProjectEnabled, isSnapshot), and have a custom constructor accepting the RemoteClusterService and calling isSnapshot().
Maybe this approach is somewhat shortsighted if we want to add more derived states or services, so take it with a grain of salt!
|
|
||
| public void testValidate_ProjectRouting_noCps() { | ||
| var setting = QuerySettings.PROJECT_ROUTING; | ||
| assumeTrue("CPS validation not enabled yet", setting.preview()); |
There was a problem hiding this comment.
I find this check dangerous, as we may forget about it, and not test it later even if it's already implemented.
This is actually a commented test under the hood; maybe we should just comment it, and add a comment with the why.
More visible than an assumption that will never be truthy
|
Thanks @ivancea! |
…review' into esql/set_command_tech_preview
3 participants
Moving the validation to the level of single settings, based on their
snapshotflag.Also doing some refactoring to inject specific validation for
project_routing, based on CPS being enabled, and to make unit tests easier to write.