-
Notifications
You must be signed in to change notification settings - Fork 197
Add support for relative URI remote group members. #5625
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
base: main
Are you sure you want to change the base?
Add support for relative URI remote group members. #5625
Conversation
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.
Thanks for the hard work, this looks tricky and I can imagine the back and forths to get it right ...
I added a few questions and a couple suggestions for fixing the tests.
resources.rest_client()->rest_legacy(), &components)); | ||
// The asset is not registered on REST, so we set the object type using | ||
// the asset storage location. | ||
if (!components.asset_storage.empty()) { |
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.
Can we have an else
case here? What if the parent group_uri
is a URI with default storage, e.g. tiledb://myws/myts/myfld1/mygroup
?
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.
I started on this today and it looks like there is at least one bug there but I haven't quite sorted it out yet. I pushed the test cases I was using to debug but they are still a WIP. The array test passes but I want to look closer, the group test is failing. I think the array test passing actually makes more sense then the group test failure that I'm seeing, but as I mentioned I still need to look closer
I will clean these up so there is less duplication :)
+ Create subgroup prior to add_member for remaining legacy REST test case
This avoids checking for an empty name server side
7f54ed1
to
8c1f2e1
Compare
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 provided that this comment is also addressed: #5625 (comment)
std::string expected_uri = | ||
build_tiledb_uri(member_uri, "groups_relative/relative_group"); | ||
CHECK(member.uri() == expected_uri); | ||
CHECK(group.is_relative("relative_group")); |
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.
Your Server PR made this code produce a relative member instead of an absolute? AFAIK the behavior that creating a group/array inside a group results in adding it as a member existed.
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.
Vlasis made a similar comment on my server side PR so I cc'd you in my reply there for some more detail. But the TLDR is yes, currently creating a group member using an asset path with a parent asset that is an existing group on REST will build a relative storage URI server side and create the new array / group member relative to its parent group on backend storage. I found that this is the case for both array and group creation routes today prior to any group member relative URI changes.
This adds support for using relative URIs when adding group members to a remote group in TileDB-Server. This support will not be added for legacy REST.
Fixes ENG-112
TYPE: FEATURE
DESC: Add support for relative URI remote group members.