PPL Alerting: PPL Dependencies and Utils#2114
PPL Alerting: PPL Dependencies and Utils#2114toepkerd merged 5 commits intoopensearch-project:mainfrom
Conversation
toepkerd
force-pushed
the
pr
branch
2 times, most recently
from
65caee5 to
5d24d90
Compare
| Setting.Property.NodeScope, Setting.Property.Dynamic | ||
| ) | ||
|
|
||
| val NOTIFICATION_SUBJECT_SOURCE_MAX_LENGTH = Setting.intSetting( |
There was a problem hiding this comment.
Is this part of a bug fix? Wouldnt adding this validation cause a breaking change?
There was a problem hiding this comment.
These checks are only made against PPL Triggers when PPL Monitors are being created, they're not applied to existing Monitor types.
There was a problem hiding this comment.
| Setting.Property.NodeScope, Setting.Property.Dynamic | ||
| ) | ||
|
|
||
| val NOTIFICATION_MESSAGE_SOURCE_MAX_LENGTH = Setting.intSetting( |
There was a problem hiding this comment.
Is this part of a bug fix? Wouldnt adding this validation cause a breaking change?
There was a problem hiding this comment.
These checks are only made against PPL Triggers when PPL Monitors are being created, they're not applied to existing Monitor types.
There was a problem hiding this comment.
toepkerd
requested review from
AWSHurneyt,
KashKondaka,
amsiglan,
bowenlan-amzn,
eirsep,
engechas,
getsaurabh02,
goyamegh,
manaswini1920,
nishtham-amazon,
rishabhmaurya,
riysaxen-amzn and
vikhy-aws
as code owners
|
|
||
| // SQL/PPL plugin dependencies are included in alerting-core | ||
| api project(":alerting-core") | ||
| implementation 'org.json:json:20240303' |
There was a problem hiding this comment.
- where are we using this and why not jackson?
- check if opensearch is providing any org.json dep
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "_meta" : { | |||
| "schema_version": 8 | |||
| } | ||
| } | ||
|
|
||
| def sqlJarDirectory = "$buildDir/dependencies/opensearch-sql-plugin" |
There was a problem hiding this comment.
Taking a dependency on all of the PPL/SQL seems intensive just to get a reference to TransportPPLQueryRequest. It also forces/couples alerting to care about the version of PPL running in the system. Do we know how other plugins are handling a dependency on PPL/SQL?
Are there further dependencies than TransportPPLQueryRequest that are needed in coming changes?
Are we able to interact with the PPL plugin without this dependency? Perhaps via a rest or HTTP request - or simply passing a generic object for the request instead of an explicit TransportPPLQueryRequest
There was a problem hiding this comment.
The approach used here models after the ml-commons's skills plugin: https://github.com/opensearch-project/skills/blob/main/build.gradle. They also use Transport calls to call SQL/PPL plugin for executing PPL queries.
In addition to TransportPPLQueryRequest, TransportPPLQueryResponse and PPLQueryAction are also needed.
Other plugins make PPL queries via REST. However, moving away from using a transport call may have consequences outside of OpenSource.
Signed-off-by: Dennis Toepker <toepkerd@amazon.com>
Signed-off-by: Dennis Toepker <toepkerd@amazon.com>
Signed-off-by: Dennis Toepker <toepkerd@amazon.com>
Signed-off-by: Dennis Toepker <toepkerd@amazon.com>
Signed-off-by: Dennis Toepker <toepkerd@amazon.com>
5 participants
Description
SQL/PPL Plugin dependencies and the interface for cross-plugin communication.
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.