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 IncludeInfoObject flag for terminating the .info in gcs as well #1082

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DarioCalovic
Copy link

Description
Issueing the termination process the filestore removes both the binary as well as the .info file. The gcc store only removes the binary file.

How to solve
A user wrote:

A solution would be to add an object IncludeInfoObject to GCSFilterParams, which can be set by the termination code to instruct FilterObjects to not exclude .info objects.

That's what i have done, if i understood the assessment correctly. Everywhere where the GCSFilterParams was declared, I included the IncludeInfoObject even if the default value would be false anyway. Only at the termination process, the IncludeInfoObject is set to true.

Side note: My very first pull request, just trying to help.

Related Issue #1075

@Acconut
Copy link
Member

Acconut commented Feb 8, 2024

Thank you very much this PR and congrats on your first contribution!

I added a test for FilterObjects with IncludeInfoObject: true, but it seems to be behaving differently than expected. The returned list only includes the .info object, but not the other objects. Maybe you could have a second look at the implementation changes for FilterObjects.

@DarioCalovic
Copy link
Author

@Acconut You are right. Could you look at the latest commit? I am not sure, if i changed the behaviour too much. First of all, i noticed, that this function always returns an empty "" on the first index. Is this intentionally? Second, i tried to simplify the validation process. Let me know what you think.

@Acconut
Copy link
Member

Acconut commented Feb 9, 2024

First of all, i noticed, that this function always returns an empty "" on the first index. Is this intentionally?

I noticed the same behavior during my tests. Unfortunately, I don't know if this is needed or not. The gcsstore was added in #106 by @tab1293 and I haven't worked much with it. Maybe he could shed some light on this :)

I am not sure, if i changed the behaviour too much.

I'll have another look

@Acconut
Copy link
Member

Acconut commented Mar 4, 2024

Thank you for the changes. To be honest, I am not 100% sure that this PR doesn't break some functionality because I am not used to working with the code for Google Cloud Storage. However, I am open to giving this a try. Have you tested your PR in a real-world setup? Do uploads still works as expected? Does an upload work properly if a file is transferred in more than 32 requests (when composing files together there is a maximum limit of 32 objects)? You can achieve that by setting a low chunk size in your tus client. For example, uploading a 1 MB file with a chunk size of 20 KB will result in 50 requests. Does the bucket on GCS include the valid file afterwards?

If all of that looks good, we can merge this PR.

@DarioCalovic
Copy link
Author

Let me do some tests, and i will come back at you.

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.

Uniform Termination Process for Store Implementations: Ensuring Consistent Removal of Binary and .info Files
2 participants