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

Renaming datastore Batch.mutation to mutations. #1306

Merged
merged 1 commit into from
Dec 22, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 18, 2015

This is towards the switch to v1beta3 where the Mutation
container no longer holds a list of insert/update/upsert/etc.
mutations (as Entity pb's) but instead holds a single one.
The list is moved onto the CommitRequest message definition.

Plan to follow up with methods to abstract away getting the
list of inserts, upserts, deletes, etc. as well as a method
for adding a new mutation of a given type to the changes
accumulator.

@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Dec 18, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 18, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Dec 18, 2015

@tseaver I opted to do less in this PR just so we could discuss. For example, I'm not sure if changes is the right name and I'm also not sure why the @property needs to be public.

@dhermes dhermes mentioned this pull request Dec 18, 2015
49 tasks
@tseaver
Copy link
Contributor

tseaver commented Dec 18, 2015

For example, I'm not sure if changes is the right name

Does the protobuf in vibeta3 have a changes field? If so that seems like the right name. Is it then a sequence of oneof mappings?

I'm also not sure why the @Property needs to be public.

Might we have made the mutation property public in order to silence linty noise about access to private members in tests?

@dhermes
Copy link
Contributor Author

dhermes commented Dec 18, 2015

There is no changes field, the structure changes as described above. The whole point though was that accumulated changes in the batch should not be too closely tied to the underlying representation. That's why I though changes was a nice name.

But, I was very much wanting to discuss, so this is good.


RE: "lint-y noises". I don't think so. We may have inherited it? We even access self._cluster._client in the bigtable stuff and pylint does not whine.

@tseaver
Copy link
Contributor

tseaver commented Dec 18, 2015

the Mutation container no longer holds a list of insert/update/upsert/etc. mutations (as Entity pb's) but instead holds a single one.

A single what?

@dhermes
Copy link
Contributor Author

dhermes commented Dec 19, 2015

Sorry I meant a single mutation. In v1beta2 we have

CommitRequest -> Mutation -> repeated fields containing Entity

whereas in v1beta3 we have

CommitRequest -> repeated field containing Mutations -> single Entity with one of the desired fields


Essentially, the Mutation used to hold the lists of mutated entities but now the CommitRequest holds a list of Mutations and a mutation just holds a single Entity (or Key) that changed.

@tseaver
Copy link
Contributor

tseaver commented Dec 21, 2015

Maybe mutations would be a better name, since it is a list of them.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 21, 2015

The reason I ditched mutations is because that is tied to the underlying implementation, though I'm happy to add it back if you think it's preferred to changes

@dhermes
Copy link
Contributor Author

dhermes commented Dec 21, 2015

@tseaver Or we could just name it commit request or something like that? Since the Batch is just tracking changes to be committed.

@tseaver
Copy link
Contributor

tseaver commented Dec 22, 2015

Given that we don't have backward-/forward compatibility cocerns: if the objects which make up the to-be-named list attribute are all of type Mutation, then mutations is as good a name as any, and better than most.

@dhermes dhermes changed the title Renaming datastore Batch.mutation to changes. Renaming datastore Batch.mutation to mutations. Dec 22, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Dec 22, 2015

@tseaver OK I updated the name. PTAL.

@tseaver
Copy link
Contributor

tseaver commented Dec 22, 2015

LGTM

This is towards the switch to `v1beta3` where the `Mutation`
container no longer holds a list of insert/update/upsert/etc.
mutations (as `Entity` pb's) but instead holds a single one.
The list is moved onto the `CommitRequest` message definition.

Plan to follow up with methods to abstract away getting the
list of inserts, upserts, deletes, etc. as well as a method
for adding a new mutation of a given type to the `changes`
accumulator.
dhermes added a commit that referenced this pull request Dec 22, 2015
Renaming datastore Batch.mutation to mutations.
@dhermes dhermes merged commit f89b76a into googleapis:master Dec 22, 2015
@dhermes dhermes deleted the datastore-rename-mutations branch December 22, 2015 17:35
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.

3 participants