-
Notifications
You must be signed in to change notification settings - Fork 144
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 method to set metadata #38
Comments
The relevant end-point appears to be (object/patch)[https://cloud.google.com/storage/docs/json_api/v1/objects/patch] with a body like |
Actually it is simple to do at file creation - see here for resumable but same in simple_upload:
The metadata can be passed as parameter in the |
OK, then I guess both should be done. |
https://cloud.google.com/storage/docs/json_api/v1/how-tos/simple-upload indicates that if metadata needs to be added, simple is no good. Actually it can be used but through a different URL call. |
You would be doubling the time for uploads on small files without metadata. Could you not stay with simple upload, but call the PATCH method after close in the case that there is metadata? |
Not sure I understand - I have no use for simple upload as none of my writes are < 5Mb. The change proposal will only affect (benefit) resumable uploads in For simple upload, The change is a very minor. Feel free to reject or bundle otherwise but it's working nicely for me. |
Sorry, I thought you meant avoiding simple upload altogether. I would like metadata added in both cases, rather than silently ignoring them in some cases. So the plan:
|
Sorry but I can only help with the 2 first bullets. |
See #39 |
@yiga2 , I'll leave this issue open despite your PR until the rest of the functionality is complete. |
@martindurant I have found a easy way to get simple_upload to work but i's using the
Since Are you fine with it or want to stick with a BTW, Google's |
Have you tried setting the same header keys with the original end-point? It would be odd if the two syntaxes had different capabilities. I don't mind particularly, but it would make the code less friendly, I suspect. |
I tried and as I wrote, it is ignored. Odd indeed and there is no indication that either endpoint will be deprecated.
The Also that's the only way to set the metadata at object creation, which saves an API call and risk of error. |
@martindurant Considering the existence of Google's abstraction layer and their move to under-the-hood gRPC, I wonder if Sure the changes are profound and beyond my knowledge. The fact that See you back on other projects I am following, fastparquet and fastavro ! |
@martindurant Would you mind reopening this issue? I've a use case that likely requires metadata on small uploads and this seems like a natural place to plan that work. |
@martindurant et al. - I am back ! Turned out I needed
Sorry I haven't touched Hope someone will take this through and close the loop. |
Thank, @yiga2 ! |
|
The On the test case, count on me ! But reflecting on this as I was playing with different chunk (block) size for optimal performance (which for GCP-to-GCP is the max 8MB), there is no real performance benefit between Hence question: what about skipping At the other hand, |
Not sure I would reopen this but if anyone is reading this claiming it doesn't work: make sure the dict values are strings as One should do as follows:
and deserialize back when reading the metadata. @martindurant not sure if wrapping this in the core code i.e. for each value, if not str, jsonify, is worth it but raising an error would be good. |
Sounds reasonable to either make this work as expected or to document for those that come across the current behaviour and would otherwise be surprised. |
Feature request (or current workaround)
dict
parameter at file creation (open
inw
mode)The text was updated successfully, but these errors were encountered: