ESQL: Support "capabilites" in the csv-spec tests #108464
Conversation
This flips the csv-spec construct called `required_feature:` from using "cluster features" to using "cluster capabilities". "Features" are a "heavy" concept that live in the cluster state and should be used for quickly checking things on the local node. "Capabilities" are fairly fluid list of strings that live on each node and are calculated on the fly so much nicer for testing. This adds all existing "cluster features" for esql as "cluster capabilities" for the ESQL `_query` and `_query/async` actions. The tests just check that. In a follow-up change I'll replace the syntax `required_feature:` with `required_capability:`. Our esql capabilities all starts with `esql.` - but capabilities are naturally scoped to the endpoint. So I've removed the `esql.` from the capabilities we add.
nik9000
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
| * Does the cluster on the other side of {@code client} support the set | ||
| * of capabilities for specified path and method. | ||
| */ | ||
| protected static boolean clusterHasCapability(RestClient client, String method, String path, Collection<String> capabilities) |
There was a problem hiding this comment.
These are general methods, so they should also have a parameter for query params too
|
The failure is quite real and looks like a bug in capabilities. Or something that makes it kind of difficult for us. If I run this one: and drop a breakpoint in
What I really want is an error every time - some of the nodes don't support the request and I'm going to need to fall back to cluster capabilities and/or just say "I can't run this test". |
|
Yes, the behaviour as-merged doesn't deal well with mixed clusters. This is improved by #108425, which I'm working on today |
|
@elasticmachine update branch |
… into esql_features_are_capabilities
There was a problem hiding this comment.
Thanks a lot @nik9000, this looks good to me (except the bug you're fighting, of course).
| @@ -222,8 +221,8 @@ public final void test() throws Throwable { | |||
| * The csv tests support all but a few features. The unsupported features | |||
| * are tested in integration tests. | |||
| */ | |||
| assumeFalse("metadata fields aren't supported", testCase.requiredFeatures.contains(EsqlFeatures.METADATA_FIELDS.id())); | |||
| assumeFalse("enrich can't load fields in csv tests", testCase.requiredFeatures.contains(EsqlFeatures.ENRICH_LOAD.id())); | |||
| assumeFalse("metadata fields aren't supported", testCase.requiredCapabilities.contains("metadata_fields")); | |||
There was a problem hiding this comment.
Nit: using constants could be more robust to future changes. We could make EsqlCapabilities.cap public, then this could just be
| assumeFalse("metadata fields aren't supported", testCase.requiredCapabilities.contains("metadata_fields")); | |
| assumeFalse("metadata fields aren't supported", testCase.requiredCapabilities.contains(cap(EsqlFeatures.METADATA_FIELDS))); |
There was a problem hiding this comment.
Yeah, I probably should do that.
Sorry, I realized this is only relevant if we disabled esql's cluster features, which we're not doing here. |
nik9000
added
the
auto-merge-without-approval
This flips the csv-spec construct called
required_feature:from using "cluster features" to using "cluster capabilities". "Features" are a "heavy" concept that live in the cluster state and should be used for quickly checking things on the local node. "Capabilities" are fairly fluid list of strings that live on each node and are calculated on the fly so much nicer for testing.This adds all existing "cluster features" for esql as "cluster capabilities" for the ESQL
_queryand_query/asyncactions. The tests just check that.In a follow-up change I'll replace the syntax
required_feature:withrequired_capability:.Our esql capabilities all starts with
esql.- but capabilities are naturally scoped to the endpoint. So I've removed theesql.from the capabilities we add.