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

Removing serializable option from datastore Transaction. #1294

Merged
merged 1 commit into from
Dec 17, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 16, 2015

This is in advance of the option being removed in v1beta3. Since in v1beta3:

All transactions are now serializable

we set the isolation to always be serializable.


@pcostell Is this "safe" to do as prep for the version upgrade?

Also, is there an equivalent replacement in v1beta3? AFAICT the isolation level is just gone.

@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Dec 16, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 16, 2015
@dhermes dhermes mentioned this pull request Dec 16, 2015
49 tasks
@pcostell
Copy link
Contributor

In v1beta3 we opted to only expose serializable which is a stronger form of isolation. I think in v1beta2 if you want to remove the option you need to force it to be set to serializable (the default is snapshot).

Also, I'm not sure if you've seen it, but here are the (whitelisted) release notes: https://cloud.google.com/datastore/v1beta3/release-notes

@tseaver
Copy link
Contributor

tseaver commented Dec 16, 2015

LGTM. I never saw the point in snapshot for a transaction, anyhow.

@tseaver
Copy link
Contributor

tseaver commented Dec 16, 2015

@pcostell that URL 404s for me.

@pcostell
Copy link
Contributor

Sorry it is behind a whitelist. Can you email me your google account that you use? (@gmail, @google, or other Google Apps domain)

@dhermes dhermes force-pushed the datastore-remove-serializable branch from 9ad1f2d to 0a407f6 Compare December 17, 2015 02:02
@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

Thanks @pcostell. Can you also add daniel.j.hermes@gmail.com? (I can see it with my corp email, but seldom have corp access.)


@tseaver After seeing Patrick's comments and the whitelist, I think it's best to set the isolation level always to serializable.

PTAL

@tseaver
Copy link
Contributor

tseaver commented Dec 17, 2015

This is in advance of the option being removed in v1beta3. Since
in v1beta3:

> All transactions are now serializable

we set the isolation to always be serializable.
@dhermes dhermes force-pushed the datastore-remove-serializable branch from 1cb8a3d to 2ceace5 Compare December 17, 2015 03:59
@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

@tseaver are we LGTM with the update to isolation_level?

@tseaver
Copy link
Contributor

tseaver commented Dec 17, 2015

LGTM: setting explicitly in the protobuf makes the behavior forward compatible.

dhermes added a commit that referenced this pull request Dec 17, 2015
Removing serializable option from datastore Transaction.
@dhermes dhermes merged commit 977b7f8 into googleapis:master Dec 17, 2015
@dhermes dhermes deleted the datastore-remove-serializable branch December 17, 2015 04:34
@pcostell
Copy link
Contributor

@dhermes @tseaver -- added your accounts to the whitelist.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2015

Thanks

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

Successfully merging this pull request may close these issues.

4 participants