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

counter_incs by incrementRows #89

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ummae
Copy link

@ummae ummae commented Apr 29, 2015

related: #35

I am not sure that is proper way, but at least it works.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.50% when pulling d455db0 on ummae:master into 6205177 on wbolster:master.

@@ -558,6 +559,25 @@ def counter_inc(self, row, column, value=1):
return self.connection.client.atomicIncrement(
self.name, row, column, value)

def counter_incs(self, row, data):
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps counters_inc is a better name? And can you move it below counter_dec for clarity?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I agree and I will move it below counter_dec.

@wbolster
Copy link
Member

wbolster commented May 3, 2015

Hi, thanks for working on this. I've added a few questions/comments.

Any chance you can add some tests?

@ummae
Copy link
Author

ummae commented May 3, 2015

Sure, and I found some conventional problem on PEP8 styles(e.g. line break 79bytes, etc..).
I will re-PR after fixing it all.

ummae added 3 commits May 3, 2015 22:08
- Renaming method counter_incs to counters_inc
- Add nose tests for counters_inc
- Renaming method counter_incs to counters_inc
- Add nose tests for counters_inc
@landscape-bot
Copy link

Code Health
Repository health increased by 0.62% when pulling 209ce27 on ummae:master into 6205177 on wbolster:master.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.67% when pulling c5761f6 on ummae:master into 6205177 on wbolster:master.

@ummae
Copy link
Author

ummae commented May 7, 2015

@wbolster Hi would you review this commit?

@@ -183,6 +183,34 @@ def test_atomic_counters():
assert_equal(10, table.counter_dec(row, column, -7))


def test_multiple_counters():
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, tests look good.

@wbolster
Copy link
Member

wbolster commented May 7, 2015

Thanks for working on this.

See comments about using a dict. Additionally, I think some mention of this in the user guide might make sense.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.67% when pulling 3ab00d2 on ummae:master into 6205177 on wbolster:master.

@ummae
Copy link
Author

ummae commented May 7, 2015

I applied eventually ;)

@landscape-bot
Copy link

Code Health
Repository health increased by 0.67% when pulling ff5c525 on ummae:master into 6205177 on wbolster:master.

@ummae
Copy link
Author

ummae commented Jul 12, 2015

@wbolster Hi, I wonder do you mind to merge ;) and one question would you have plan happybase version2 for thrift2 protocol

@wbolster
Copy link
Member

Hi, #96 also adds support for batch counter increments using a different API. I think the API proposed in #96 is friendlier for applications. Thoughts?

@philip-sterne
Copy link

Hi @wbolster, I'm happy to add tests for #96 if you prefer the different API? We tried to keep the api identical to the Batch idea.

@RajatGoyal
Copy link

Any progress on this, It would be great If i can increase counters in bulk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants