-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Add extension point for extra plan checks #128840
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
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
mark-vieira
left a comment
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 left some minor comments that I think are just superfluous boilerplate that was likley copy/pasted from some other plugin. This "example" checker plugin will probably eventually get removed anyway when we introduce the "real" things so it's not critical. Otherwise LGTM.
| /** | ||
| * Marker plugin to enable {@link Verifier.ExtraCheckers}. | ||
| */ | ||
| public class ExtraCheckersPlugin extends Plugin {} |
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.
Is this just because ES requires our plugin to include something that extends Plugin even if we're just providing stuff loaded by SPI?
| @@ -0,0 +1,29 @@ | |||
| apply plugin: 'elasticsearch.internal-es-plugin' | |||
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 we should actually use elasticsearch.base-internal-es-plugin here, since this is a "test" plugin and not intended to be published or included in Elasticsearch.
| } | ||
|
|
||
| tasks.named('javaRestTest') { | ||
| usesDefaultDistribution("to be triaged") |
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.
Let's make a best effort to not use the default distribution here. We should avoid adding more tests that use this.
| tasks.named('javaRestTest') { | ||
| usesDefaultDistribution("to be triaged") | ||
| maxParallelForks = 1 | ||
| jvmArgs('--add-opens=java.base/java.nio=ALL-UNNAMED') |
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.
Why is this required?
| dependencies { | ||
| compileOnly project(':x-pack:plugin:esql') | ||
| compileOnly project(':x-pack:plugin:esql-core') | ||
| clusterPlugins project(':x-pack:plugin:esql:qa:server:extra-checkers') |
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 should be unnecessary. The internal-es-plugin plugin should automatically add this.
| .distribution(DistributionType.DEFAULT) | ||
| .setting("xpack.security.enabled", "false") | ||
| .setting("xpack.license.self_generated.type", "trial") | ||
| .shared(true) |
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 unnecessary. There's only one test in this project.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Adds an extension point so other plugins can add checks that verifier tasks that aren't part of the ESQL plugin.