Route getNotificationConfigs through SdkClient - #1225
Conversation
| null | ||
| } | ||
| } | ||
| return response.responses.mapNotNull { parseNotificationConfigDoc(it.id, it.response) } |
There was a problem hiding this comment.
Is the existing parse method not able to handle the search response?
There was a problem hiding this comment.
currently parseNotificationConfigDoc takes a GetResponse, but the search returns SearchHit objects. The SearchHit has sourceAsString but not a GetResponse, so we can't pass it directly
AWSHurneyt
left a comment
AWSHurneyt
left a comment
There was a problem hiding this comment.
Could you add unit/integ tests?
Also, it looks like the github workflows are failing for this PR. Could you take a look?
| .timeout(TimeValue(PluginSettings.operationTimeoutMs, TimeUnit.MILLISECONDS)) | ||
| .query(QueryBuilders.idsQuery().addIds(*ids.toTypedArray())) | ||
| .size(ids.size) | ||
| val searchRequest = SearchDataObjectRequest.builder() |
There was a problem hiding this comment.
I see that getAllNotificationConfigs adds the tenantId to the search request. Is it intentional to exclude that from this search request?
| val sourceBuilder = SearchSourceBuilder() | ||
| .timeout(TimeValue(PluginSettings.operationTimeoutMs, TimeUnit.MILLISECONDS)) | ||
| .query(QueryBuilders.idsQuery().addIds(*ids.toTypedArray())) | ||
| .size(ids.size) |
There was a problem hiding this comment.
This is probably an unlikely edge case, but do we need to worry about ids.size being greater than 10k in this situation? Or does searchDataObjectAsync not have that limitation?
77f54fb to
265bd10
Compare
| distributionPath=wrapper/dists | ||
| distributionSha256Sum=16f2b95838c1ddcf7242b1c39e7bbbb43c842f1f1a1a0dc4959b6d4d68abcac3 | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-9.2.0-all.zip | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-9.4.1-all.zip |
There was a problem hiding this comment.
wouldnt there be a new gradle wrapper jar?
There was a problem hiding this comment.
@MohammedAghil could you take a look at this gradle bump PR for the alerting plugin? you may be able to just copy those changes.
https://github.com/opensearch-project/alerting/pull/2122/changes
…K compatibility (opensearch-project#1224) Replace MultiGetRequest with SearchDataObjectRequest using idsQuery to ensure bulk config fetches go through SdkClient, consistent with all other operations. - Added tenantId to search request for multi-tenancy support - Updated gradle wrapper to 9.4.1 (required by OpenSearch gradle plugin) Signed-off-by: Mohammed Aghil Puthiyottil <57040494+MohammedAghil@users.noreply.github.com>
265bd10 to
53eb0d5
Compare
5 participants
Description
Route getNotificationConfigs through SdkClient by replacing MultiGetRequest with SearchDataObjectRequest using idsQuery
This is the only remaining operation in
NotificationConfigIndexthat bypassed the Remote Metadata SDK, going directly to the local OpenSearch client. The send notification flow calls this method to bulk-fetch channel configs before sending notifications, which fails when using a remote metadata backend with other metadata stores.The fix is consistent with how
getAllNotificationConfigsalready performs searches throughSdkClient.searchDataObjectAsync().Related Issues
Resolves #1224
Build Run
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.