Skip to content
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

Add possibility to tag objects. #426

Merged
merged 1 commit into from
Dec 20, 2021
Merged

Add possibility to tag objects. #426

merged 1 commit into from
Dec 20, 2021

Conversation

csc-felipe
Copy link
Contributor

@csc-felipe csc-felipe commented Dec 9, 2021

Description

This is a continuation to the tagging buckets feature from PR #419. The implementation is very similar.

The openApi spec supports tuples in 3.1 with prefixItems. We may want to change the api docs when that comes into use.

It does only one request to fetch metadata for all objects in a bucket.

NB: Please, check the Finnish language :)

Related issues

Is #5 solved with this?

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

Added backend for updating metadata for an object. Added UI for displaying tags for objects, as well as hiding them. There is a new button, called 'Edit', which makes it possible to edit an object's tags.

image
image

Previously, the api docs for container update metadata had gone to the wrong place. The fix is in this PR :)

Testing

  • Unit Tests
  • Integration Tests

Mentions

@csc-felipe csc-felipe requested review from sampsapenna and blankdots and removed request for sampsapenna December 9, 2021 07:49
@csc-felipe csc-felipe self-assigned this Dec 9, 2021
Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

The openApi spec supports tuples in 3.1 with prefixItems. We may want to change the api docs when that comes into use.

Add an issue to keep track of this.

It does only one request to fetch metadata for all objects in a bucket.

How does this perform with a lot of objects ?

We sometimes use container sometimes bucket in the API specs. We should be a bit more consistent.

Other than that LGTM

parameters:
- name: container
in: query
description: The container which metadata will be updated.
description: The container which contains the objects to be updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: The container which contains the objects to be updated.
description: The container which consists of the objects to be updated.

@csc-felipe
Copy link
Contributor Author

@sampsapenna had offered to stress test with lots of objects and containers. Could you check it out, please?

I would expect that it takes a few seconds to load the tags, similar to how it takes a few seconds to load the object table.

Created a couple of issues:

@sampsapenna
Copy link
Member

sampsapenna commented Dec 9, 2021

@sampsapenna had offered to stress test with lots of objects and containers. Could you check it out, please?

I would expect that it takes a few seconds to load the tags, similar to how it takes a few seconds to load the object table.

Created a couple of issues:

Did some stress testing – we'll have to change the existing implementation of objects in query string if we want to fetch the whole container with one request. Depending on the object names it smashes through the URL size caps even with as few as 200 containers. Alternatively splitting the requests up is also an option. The performance is good until it runs into the URL issues. After the change takes place I can test it with a couple worst case scenarios.

8192 bytes is apparently the default limit for aiohttp – 2048, however, is the recommended cap.

@sampsapenna
Copy link
Member

sampsapenna commented Dec 9, 2021

Unfortunately the Openstack API specification caps the object name to 1024 characters, so that means we can't reliably put even two objects into one request if we want to stay under the 2048 mark. Chrome is fine with 32KiB of content in the address, Firefox can deal with even more – so from the browser side we can go with higher assumptions without an issue.

I'd guess that the HAProxy in use is configured to complain sooner, just like aiohttp does.

@csc-felipe
Copy link
Contributor Author

Thanks for testing, @sampsapenna. I did think this might be an issue. The most natural for me here would be to move objects to the body, but it's usually better not to change the API, so I'll split the requests, checking for the lower 2048 limit from aiohttp.

@csc-felipe csc-felipe force-pushed the feature/object-tags branch 3 times, most recently from 5a30e46 to ba97c70 Compare December 10, 2021 13:11
csc-felipe added a commit that referenced this pull request Dec 10, 2021
@csc-felipe csc-felipe force-pushed the feature/object-tags branch 2 times, most recently from 8f7e098 to 58435db Compare December 10, 2021 13:52
@csc-felipe csc-felipe mentioned this pull request Dec 10, 2021
2 tasks
@csc-felipe
Copy link
Contributor Author

Noticed a few issues while implementing the filtering. Edit button was showing in the folder list, folder with tag, object tags not properly deleting all the tags, leaving a tag with empty value because of inconsistency in the swift backend.

Hopefully all issues are gone now.

@csc-felipe csc-felipe requested a review from blankdots December 10, 2021 15:02
@csc-felipe
Copy link
Contributor Author

This looks like the same feature #85.

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

we need to display a message, in a different PR, that a title is not editable at the moment.

@blankdots blankdots linked an issue Dec 14, 2021 that may be closed by this pull request
This is a continuation to the tagging buckets feature from PR #419.
The openApi spec supports tuples in 3.1 with prefixItems. We may want to change the api docs when that comes into use.
csc-felipe added a commit that referenced this pull request Dec 15, 2021
@blankdots
Copy link
Contributor

blankdots commented Dec 15, 2021

i think commit e147318 is from PR #429 , or looks like it

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Dec 15, 2021

think commit e147318 is from PR #429, or looks like it

I made a branch demo/tags with commits from this PR and the filtering PR to use in a demo. That filtering commit was not pushed to this PR. The message here was that this PR was referenced in that same filter commit on the other branch.

csc-felipe added a commit that referenced this pull request Dec 15, 2021
Copy link
Member

@sampsapenna sampsapenna left a comment

Choose a reason for hiding this comment

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

Tagging functionality works nicely, and does not prevent UI usage. Would be nice to be able to cancel the tag fetch when changing routes, to prevent performance issues with the filled event loop. Not sure how easily this would be implemented, though.

@csc-felipe
Copy link
Contributor Author

Would be nice to be able to cancel the tag fetch when changing routes, to prevent performance issues with the filled event loop.

I will investigate.

@csc-felipe csc-felipe merged commit d525f60 into devel Dec 20, 2021
@csc-felipe csc-felipe deleted the feature/object-tags branch December 20, 2021 12:18
csc-felipe added a commit that referenced this pull request Dec 20, 2021
csc-felipe added a commit that referenced this pull request Dec 20, 2021
@sampsapenna sampsapenna mentioned this pull request Jan 12, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using metadata to organize container files
3 participants