Use SdkClient for persistence in 4 transport actions#2060
Conversation
033be42 to
64ca2df
Compare
alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteMonitorAction.kt
Show resolved
Hide resolved
64ca2df to
ec7c4c7
Compare
alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteMonitorAction.kt
Outdated
Show resolved
Hide resolved
eirsep
left a comment
There was a problem hiding this comment.
we need to ensure we finish loop after listener.onResponse/Failure statements.
and transversely we need to make sure listener.onResponse/Failure statements. is present everywhere
we have had a lot of pain with this in past ... plz add unit tests with 100% coverage for action classes.
rest changes are LGTM
Fixed both issues:
|
16e447d to
5b2292b
Compare
alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteMonitorAction.kt
Show resolved
Hide resolved
| actionListener.onFailure( | ||
| AlertingException.wrap( |
There was a problem hiding this comment.
Do we no longer need to notify the action listener or wrap the exception type as alerting with the remote SDK?
There was a problem hiding this comment.
We still do — getMonitor() throws on failure, which is caught by resolveUserAndStart()'s catch block (L116) where actionListener.onFailure(AlertingException.wrap(t)) is called. The previous code called
onFailure directly in getMonitor() but didn't return after, causing an NPE on the next line. Moving to throw/catch fixes that while keeping the same listener notification and AlertingException wrapping.
| } | ||
| try { | ||
| val searchResponse = response.searchResponse() | ||
| if (searchResponse == null) { |
There was a problem hiding this comment.
When would the search response be null? Wondering if this should be an error case rather than defaulting to an empty list
There was a problem hiding this comment.
SDK's SearchDataObjectResponse.searchResponse() returns nullable. It can be null if the SDK fails to deserialize the response from the storage backend. Returning empty is defensive. Happy to change to an error if you think that's better for debugging.
There was a problem hiding this comment.
Got it - that sounds like an error case to me but we can reevaluate later
Given it's just a list of destinations it should be fine to have an empty list for now
|
|
||
| private suspend fun getMonitor(): Monitor { | ||
| val getRequest = GetRequest(ScheduledJob.SCHEDULED_JOBS_INDEX, monitorId) | ||
| val tenantId = client.threadPool().threadContext.getHeader(AlertingPlugin.TENANT_ID_HEADER) |
There was a problem hiding this comment.
What is the behavior if this header is not present?
There was a problem hiding this comment.
If the header is absent, tenantId is null. In local mode, multi-tenancy is disabled in the SDK, so null tenantId is fine — no tenant filtering is applied. In remote mode, the upstream service always sets the
x-tenant-id header, so it's never absent. If it were absent with multi-tenancy enabled, the SDK would reject the request with a 400 error
…ons, SearchComments Replace direct client.get/search calls with sdkClient equivalents: - TransportDeleteMonitorAction: sdkClient.getDataObject() in getMonitor() - TransportSearchMonitorAction: sdkClient.searchDataObjectAsync() in search() - TransportGetDestinationsAction: sdkClient.searchDataObjectAsync() in search() - TransportSearchAlertingCommentAction: sdkClient.searchDataObjectAsync() in search() Walk full cause chain for IndexNotFoundException handling. Add unit tests for SearchMonitor and GetDestinations SDK paths. Signed-off-by: Manaswini Ragamouni <ragamanu@amazon.com>
5b2292b to
0cce1e4
Compare
alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteMonitorAction.kt
Outdated
Show resolved
Hide resolved
a988701 to
f4e8ee5
Compare
Signed-off-by: Manaswini Ragamouni <ragamanu@amazon.com>
f4e8ee5 to
6800612
Compare
| } | ||
| try { | ||
| val searchResponse = response.searchResponse() | ||
| if (searchResponse == null) { |
There was a problem hiding this comment.
Got it - that sounds like an error case to me but we can reevaluate later
Given it's just a list of destinations it should be fine to have an empty list for now
c622dc4
into
opensearch-project:remote-metadata-support
Description
Use SdkClient for persistence in 4 transport actions:
Walk full cause chain for IndexNotFoundException handling.
Add unit tests for SearchMonitor and GetDestinations SDK paths.
Related Issues
Follows pattern from #2053
Check List
--signoff.