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

allow put and get to work with compressed blob properties #175

Merged
merged 1 commit into from
Aug 27, 2019
Merged

allow put and get to work with compressed blob properties #175

merged 1 commit into from
Aug 27, 2019

Conversation

cguardia
Copy link
Contributor

From #160.

@cguardia cguardia requested a review from chrisrossi August 27, 2019 07:59
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 27, 2019
@cguardia cguardia added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 27, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 27, 2019
@eyalhei
Copy link

eyalhei commented Aug 27, 2019

Hi, hope it is ok, I had a look.
In case someone changes the compressed flag it is not supported, is that intentional?
In the old version the compression info was somehow saved in the datastore

@chrisrossi
Copy link
Contributor

In case someone changes the compressed flag it is not supported, is that intentional?

True. See below.

In the old version the compression info was somehow saved in the datastore

I looked into this some this morning. The way NDB did this previously was by setting a "meaning uri" field of the protocol buffer to a custom value that was understood by NDB to mean it was a compressed property. Unfortunately, the modern incarnation of Datastore does not have a "meaning uri" field we can set.

So, yes, currently the way NDB knows a value stored in Datastore is compressed is from the Property definition in the Python code. There's no information stored in Datastore to indicate that a property is compressed or not. The consequence of this is that, as you point out, you can't really change a property between compressed and uncompressed without introducing errors when trying to read back out previously written values.

If this is particularly troublesome, we could look into storing BlobProperty values in some kind of wrapper that indicates compressed or uncompressed state. That could get complicated trying to preserve backwards compatibility.

Copy link
Contributor

@chrisrossi chrisrossi left a comment

Choose a reason for hiding this comment

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

This is fine with caveats noted in comments.

@andrewsg Should we open an issue for the caveats explored in the comments?

@eyalhei
Copy link

eyalhei commented Aug 27, 2019

@chrisrossi Thanks for the quick response.
The support for changing is not critical for me, as I have migration scripts as it is so that will be another case.
When do you think there will be a version with this?

Thanks

@cguardia cguardia merged commit 1173a00 into googleapis:master Aug 27, 2019
@andrewsg
Copy link
Contributor

Hi @eyalhei, this PR will be out in a new version very soon.

As far as the compressed tag caveats, let's not open an issue for it yet; I will make a note to raise this question to the Datastore team internally in our meeting next month and see what they think of it.

@eyalhei
Copy link

eyalhei commented Aug 27, 2019

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants