addressing client changes due to adding tenantId in the apis #3474
Conversation
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
dhrubo-os
requested review from
b4sjoo,
mingshl,
jngz-es,
model-collapse,
rbhavna,
ylwu-amzn,
zane-neo,
Zhangxunmt,
austintlee,
HenryL27 and
xinyual
as code owners
dhrubo-os
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dhrubo-os
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3474 +/- ##
============================================
+ Coverage 80.24% 80.25% +0.01%
- Complexity 6904 6906 +2
============================================
Files 610 610
Lines 30081 30077 -4
Branches 3370 3368 -2
============================================
Hits 24139 24139
+ Misses 4491 4486 -5
- Partials 1451 1452 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
| @@ -480,8 +480,7 @@ default ActionFuture<DeleteResponse> deleteAgent(String agentId) { | |||
| * @param listener a listener to be notified of the result | |||
| */ | |||
| default void deleteAgent(String agentId, ActionListener<DeleteResponse> listener) { | |||
| PlainActionFuture<DeleteResponse> actionFuture = PlainActionFuture.newFuture(); | |||
| deleteAgent(agentId, null, actionFuture); | |||
| deleteAgent(agentId, null, listener); | |||
There was a problem hiding this comment.
will it trigger BWC issue by using a listener?
There was a problem hiding this comment.
nope. Please check other apis.
| @@ -361,7 +361,7 @@ default ActionFuture<MLUndeployModelsResponse> undeploy(String[] modelIds, @Null | |||
| * Undeploy model | |||
| * For additional info on deploy, refer: https://opensearch.org/docs/latest/ml-commons-plugin/api/model-apis/undeploy-model/ | |||
| * @param modelIds the model ids | |||
| * @param modelIds the node ids. May be null for all nodes. | |||
| * @param nodeIds the node ids. May be null for all nodes. | |||
There was a problem hiding this comment.
"May be null for all nodes" - this sounds not a description but a suggestion. Why not just put "Use null for all nodes" if you are fully sure.
There was a problem hiding this comment.
I didn't change anything in the description. It was like that before. I just fixed nodeIds, before it was modelIds
We already tested it before with other apis like for model (which is mostly being used in other dependencies like skills/FF). If the client get the updated client from their maven snapshot, then it won't cause any issue, but before that it might cause some integ failure. But maven snapshot updates daily, so we should be fine here. |
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit 17b4d74)
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit 17b4d74)
Description
[addressing client changes due to adding tenantId in the apis]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.