adding multi-tenancy + sdk client related changes to model, model group and connector update #3399
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25a0a94 to
cb76c34
Compare
|
UT failed? |
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
pyek-bot
reviewed
Jan 18, 2025
| } | ||
|
|
||
| @Test | ||
| public void serialization_withOlderVersion_Success() throws IOException { |
There was a problem hiding this comment.
what do you think about adding a test for mixed versions for bwc?
Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
dhrubo-os
commented
Jan 20, 2025
| * @param excludes fields excluded | ||
| * @param listener action listener | ||
| */ | ||
| public void getModel(String modelId, String tenantId, String[] includes, String[] excludes, ActionListener<MLModel> listener) { |
There was a problem hiding this comment.
@arjunkumargiri check this method
arjunkumargiri
approved these changes
Jan 20, 2025
ylwu-amzn
approved these changes
Jan 20, 2025
jngz-es
reviewed
Jan 21, 2025
Comment on lines
+149
to
+153
| if (source.get(FUNCTION_NAME_FIELD) != null) { | ||
| algorithmName = source.get(FUNCTION_NAME_FIELD).toString(); | ||
| } else if (source.get(ALGORITHM_FIELD) != null) { | ||
| algorithmName = source.get(ALGORITHM_FIELD).toString(); | ||
| } |
There was a problem hiding this comment.
I am confused about these two fields FUNCTION_NAME_FIELD and ALGORITHM_FIELD, looks like they are both to set algorithm name.
There was a problem hiding this comment.
This is same what we had before:
if (getResponse.getSource() != null && getResponse.getSource().get(ALGORITHM_FIELD) != null) {
algorithmName = getResponse.getSource().get(ALGORITHM_FIELD).toString();
}
Comment on lines
+156
to
+158
| if (!TenantAwareHelper | ||
| .validateTenantResource(mlFeatureEnabledSetting, tenantId, mlModel.getTenantId(), actionListener)) { | ||
| return; |
There was a problem hiding this comment.
The tenanId is included in the getDataObjectRequest, may we not need this validation?
There was a problem hiding this comment.
We need to have this validation to make sure, one tenant is not accessing other tenant's resources.
| RestStatus.BAD_REQUEST | ||
| ) | ||
| ); | ||
| if (isModelNotDeployed(mlModelState)) { |
There was a problem hiding this comment.
Should it be else if () to replace the above else?
There was a problem hiding this comment.
I didn't clearly follow you here.
jngz-es
approved these changes
Jan 21, 2025
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Jan 21, 2025
…up and connector update (#3399) * adding multi-tenancy + sdk client related changes to model, model group and connector update Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed more comments + refactored few codes Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit f63b961)
dhrubo-os
added a commit
to dhrubo-os/ml-commons
that referenced
this pull request
Jan 21, 2025
…up and connector update (opensearch-project#3399) * adding multi-tenancy + sdk client related changes to model, model group and connector update Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed more comments + refactored few codes Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
dhrubo-os
added a commit
to dhrubo-os/ml-commons
that referenced
this pull request
Jan 21, 2025
…up and connector update (opensearch-project#3399) * adding multi-tenancy + sdk client related changes to model, model group and connector update Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed more comments + refactored few codes Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
dhrubo-os
added a commit
to dhrubo-os/ml-commons
that referenced
this pull request
Jan 21, 2025
…up and connector update (opensearch-project#3399) * adding multi-tenancy + sdk client related changes to model, model group and connector update Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed comments Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> * addressed more comments + refactored few codes Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> --------- Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>
5 tasks
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
[adding multi-tenancy + sdk client related changes to model, model group and connector update]
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.