-
Notifications
You must be signed in to change notification settings - Fork 129
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.
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
adding tenantId field in model group index #2862
adding tenantId field in model group index #2862
Conversation
Signed-off-by: Dhrubo Saha <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit.
@@ -58,7 +58,7 @@ public class CommonValue { | |||
public static final String ML_MODEL_GROUP_INDEX = ".plugins-ml-model-group"; | |||
public static final String ML_MODEL_INDEX = ".plugins-ml-model"; | |||
public static final String ML_TASK_INDEX = ".plugins-ml-task"; | |||
public static final Integer ML_MODEL_GROUP_INDEX_SCHEMA_VERSION = 2; | |||
public static final Integer ML_MODEL_GROUP_INDEX_SCHEMA_VERSION = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self as a reminder. When we eventually get this on main we'll have to make sure there aren't any other conflicting schema updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
+ " }\n" | ||
+ " }\n" | ||
+ " },\n" | ||
+ " \"" + TENANT_ID + "\" : {\"type\": \"keyword\"},\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works 'cause it's valid JSON, but all the other strings (see MlModelGroup.ACCESS
right below) do the "pretty" JSON equivalent. Might want to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
"took": 11,
"timed_out": false,
"_shards": {
"total": 1,
"successful": 1,
"skipped": 0,
"failed": 0
},
"hits": {
"total": {
"value": 1,
"relation": "eq"
},
"max_score": 1.0,
"hits": [
{
"_index": ".plugins-ml-model-group",
"_id": "k7BBoJEBmupm26gNRc4D",
"_score": 1.0,
"_source": {
"name": "Cohere embedding normal",
"latest_version": 1,
"description": "embedding sagemaker model",
"access": "public",
"created_time": 1724970517643,
"last_updated_time": 1724970517930,
"tenant_id": "1234567"
}
}
]
}
}
This is mapping I got after making this change. Seems like similar?
b1be05e
into
opensearch-project:feature/multi_tenancy
Description
[Describe what this change achieves]
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.