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

Find a better name for Row.commit_modifications() or fold the behavior into #1548

Closed
dhermes opened this issue Feb 29, 2016 · 8 comments
Closed
Assignees
Labels
api: bigtable Issues related to the Bigtable API. type: question Request for information or clarification. Not an issue.

Comments

@dhermes
Copy link
Contributor

dhermes commented Feb 29, 2016

See discussion in #1547 for context.

The name modifications is intended to map to the RPC method name ReadModifyWriteRow but the word modification is a synonym of mutation, so this method is poorly named. The method actually sends changes that

  • increment an int (stored in BT as 8 bytes)
  • append bytes to a cell value

so we should either

  • Fold it into Row.commit() as commit(append_only=True)
  • Rename it to something like Row.commit_cell_updates (or a better name than that)
@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Feb 29, 2016
@dhermes dhermes self-assigned this Feb 29, 2016
@dhermes dhermes added the type: question Request for information or clarification. Not an issue. label Feb 29, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Feb 29, 2016

@jonparrott @jgeewax @tseaver I'd love to have you all chime in.

@theacodes
Copy link
Contributor

I like the first option, but unsure about complexity/edge cases.

For option two, would commit_append be a better name?

@jgeewax
Copy link
Contributor

jgeewax commented Feb 29, 2016

I like the first also. commit() does all commits, commit(append_only=True) does just the appending commits. Seems super clear to me.

@tseaver
Copy link
Contributor

tseaver commented Feb 29, 2016

What does the user expect the state-of-the-world to be after commit(append_only=True), assuming there were any pending non-append modifications?

@dhermes
Copy link
Contributor Author

dhermes commented Feb 29, 2016

I think the best way forward is to actually create Row(..., append=True) and then make it so that in the append case, only append_cell_value and increment_cell_value will work.

This way Row.commit() does have to worry about sending up to 3 API requests to the RPC methods MutateRow (no filter), CheckAndMutateRow (filter) and ReadModifyWriteRow (append).


No issues with complexity, just worried that people would be confused. Here's some code snippets for how things work right now

>>> row1 = table.row(b'row-key')
>>> row2 = table.row(b'row-key', filter_=filter_)
>>>
>>> row1.set_cell(u'fam', b'col', b'val')
>>> row2.set_cell(u'fam', b'col', b'val')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "gcloud/bigtable/row.py", line 166, in set_cell
    self._get_mutations(state).append(mutation_pb)
  File "gcloud/bigtable/row.py", line 97, in _get_mutations
    raise ValueError('A filter is set on the current row, but no '
ValueError: A filter is set on the current row, but no state given for the mutation
>>>
>>> # Use state
>>> row1.set_cell(u'fam', b'col', b'val', state=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "gcloud/bigtable/row.py", line 166, in set_cell
    self._get_mutations(state).append(mutation_pb)
  File "gcloud/bigtable/row.py", line 102, in _get_mutations
    raise ValueError('No filter was set on the current row, but a '
ValueError: No filter was set on the current row, but a state was given for the mutation
>>> row2.set_cell(u'fam', b'col', b'val', state=True)
>>>
>>> # state also an argument in delete(), delete_cell(), delete_cells()
>>>
>>> row1.append_cell_value(u'fam', b'col', b'extra-bytes')
>>> row2.append_cell_value(u'fam', b'col', b'extra-bytes')
>>>
>>> row1.increment_cell_value(u'fam', b'col', 123)
>>> row2.increment_cell_value(u'fam', b'col', 123)

@dhermes
Copy link
Contributor Author

dhermes commented Feb 29, 2016

@tseaver Having a hard time answering your question. Can you give a scenario of what you have in mind?

@tseaver
Copy link
Contributor

tseaver commented Feb 29, 2016

If commit() doesn't take the append_only argument (which I thought was the option being preferred), then my question is moot.

[For rows created with append_only], only append_cell_value and increment_cell_value will work.

That seems pretty natural: the user can't get the row into a state where uncommitted changes remain after a call to commit().

@dhermes
Copy link
Contributor Author

dhermes commented Feb 29, 2016

Yup. Will implement and send out for PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

4 participants