Skip to content

Route getNotificationConfigs through SdkClient #1225

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.

Sign up for GitHub

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

Open
MohammedAghil wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Route getNotificationConfigs through SdkClient #1225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions notifications/gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionSha256Sum=16f2b95838c1ddcf7242b1c39e7bbbb43c842f1f1a1a0dc4959b6d4d68abcac3
distributionUrl=https\://services.gradle.org/distributions/gradle-9.2.0-all.zip
distributionSha256Sum=708d2c6ecc97ca9a11838ef64a6c2301151b8dd10387e22dc1a12c30557cab5b
distributionUrl=https\://services.gradle.org/distributions/gradle-9.4.1-all.zip

@eirsep eirsep May 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldnt there be a new gradle wrapper jar?

@AWSHurneyt AWSHurneyt May 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

networkTimeout=10000
validateDistributionUrl=true
zipStoreBase=GRADLE_USER_HOME
Expand Down
39 changes: 32 additions & 7 deletions ...tifications/src/main/kotlin/org/opensearch/notifications/index/NotificationConfigIndex.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import org.opensearch.action.admin.indices.mapping.put.PutMappingRequest
import org.opensearch.action.bulk.BulkResponse
import org.opensearch.action.delete.DeleteResponse
import org.opensearch.action.get.GetResponse
import org.opensearch.action.get.MultiGetRequest
import org.opensearch.action.get.MultiGetResponse
import org.opensearch.action.index.IndexResponse
import org.opensearch.action.search.SearchResponse
import org.opensearch.action.support.clustermanager.AcknowledgedResponse
Expand Down Expand Up @@ -212,12 +210,39 @@ internal object NotificationConfigIndex : ConfigOperations {
*/
override suspend fun getNotificationConfigs(ids: Set<String>): List<NotificationConfigDocInfo> {
createIndex()
val getRequest = MultiGetRequest()
ids.forEach { getRequest.add(INDEX_NAME, it) }
val response: MultiGetResponse = client.suspendUntilTimeout(PluginSettings.operationTimeoutMs) {
multiGet(getRequest, it)
val sourceBuilder = SearchSourceBuilder()
.timeout(TimeValue(PluginSettings.operationTimeoutMs, TimeUnit.MILLISECONDS))
.query(QueryBuilders.idsQuery().addIds(*ids.toTypedArray()))
.size(ids.size)

@AWSHurneyt AWSHurneyt May 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

val searchRequest = SearchDataObjectRequest.builder()

@AWSHurneyt AWSHurneyt May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that getAllNotificationConfigs adds the tenantId to the search request. Is it intentional to exclude that from this search request?

.indices(INDEX_NAME)
.tenantId(currentTenantId())
.searchSourceBuilder(sourceBuilder)
.build()
val response: SearchResponse = sdkClient.suspendUntilTimeout(PluginSettings.operationTimeoutMs) {
sdkClient.searchDataObjectAsync(searchRequest).whenComplete(it)
}
return response.hits.hits.mapNotNull { hit ->
try {
val parser = XContentType.JSON.xContent().createParser(
NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE,
hit.sourceAsString
)
parser.nextToken()
val doc = NotificationConfigDoc.parse(parser)
val info = DocInfo(
id = hit.id,
version = hit.version,
seqNo = hit.seqNo,
primaryTerm = hit.primaryTerm
)
NotificationConfigDocInfo(info, doc)
} catch (e: Exception) {
log.warn("$LOG_PREFIX:getNotificationConfigs - failed to parse ${hit.id}: ${e.message}")
null
}
}
return response.responses.mapNotNull { parseNotificationConfigDoc(it.id, it.response) }

@engechas engechas May 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the existing parse method not able to handle the search response?

@MohammedAghil MohammedAghil May 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand Down