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

Implementing HappyBase Table atomic counter helpers. #1514

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 21, 2016

No description provided.

@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Feb 21, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 21, 2016
"""
row = self._low_level_table.row(row)
if isinstance(column, six.binary_type):
column = column.decode('utf-8')

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 21, 2016

@jgeewax HappyBase also has a counter_set but as of now, I have made it NotImplemented because Bigtable doesn't have a way to make it atomic. WDYT is the right move here? I did make a note in the docstring of counter_get that

.. note::

    Application code should **never** store a counter value directly;
    use the atomic :meth:`counter_inc` and :meth:`counter_dec` methods
    for that.

@tseaver
Copy link
Contributor

tseaver commented Feb 22, 2016

@dhermes, @jgeewax: I'm pretty much offline until Thursday, 2016-02-25 (in Palo Alto teaching a course). Likewise for #1515, #1516, #1502, #1512, and #1513.

@dhermes dhermes assigned theacodes and unassigned tseaver Feb 22, 2016
@theacodes
Copy link
Contributor

This LGTM, although from a readability nit standpoint counter_inc is a little hard to follow at first glance. I'm unsure why.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2016

RE: "Hard to follow." That's the kind of feedback I was hoping to surface (by breaking into small PRs). Do you think the low-level row should handle more of the responsibilities? Do you think the method should use a helper? etc.

@theacodes
Copy link
Contributor

Perhaps just some comments in-line. Poor example, but what is bytes_value = column_cells[0][0] doing exactly? Why [0][0]?

@theacodes
Copy link
Contributor

(this also just may be my unfamiliarity with bigtable showing)

column_cells = modified_cells[column_family_id][column_qualifier]
if len(column_cells) != 1:
raise ValueError('Expected server to return one modified cell.')
bytes_value = column_cells[0][0]

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2016

I don't think it's your unfamiliarity, but that also is good in that you don't have a bias towards how things should be. I had to look up what [0][0] is for: Row.commit_modifications returns a dictionary keyed by column families. Each column family contains a dictionary keys by column names (within the family). Each column has a list of tuples. Each tuple is a pair of the bytes in the cell and the associated timestamp. It's enough to need a breather after the explanation.

@theacodes
Copy link
Contributor

Wow, that is a lot to explain.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2016

Luckily the docstring proves that a picture is worth a thousand words:

{
    u'col-fam-id': {
        b'col-name1': [
            (b'cell-val', datetime.datetime(...)),
            (b'cell-val-newer', datetime.datetime(...)),
        ],
        b'col-name2': [
            (b'altcol-cell-val', datetime.datetime(...)),
        ],
    },
    u'col-fam-id2': {
        b'col-name3-but-other-fam': [
            (b'foo', datetime.datetime(...)),
        ],
    },
}

@theacodes
Copy link
Contributor

Ah, very helpful. Can you include a comment that notes the expected format of the column_cells object?

@dhermes
Copy link
Contributor Author

dhermes commented Feb 24, 2016

@jonparrott PTAL

@theacodes
Copy link
Contributor

LGTM, please squash commits.

dhermes added a commit that referenced this pull request Feb 24, 2016
Implementing HappyBase Table atomic counter helpers.
@dhermes dhermes merged commit 983d0ee into googleapis:master Feb 24, 2016
@dhermes dhermes deleted the happybase-atomic-counter branch February 24, 2016 17:43
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. 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