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

Add SendBatch API #202

Merged
merged 4 commits into from
Jul 27, 2023
Merged

Add SendBatch API #202

merged 4 commits into from
Jul 27, 2023

Conversation

aaronbee
Copy link
Collaborator

@aaronbee aaronbee commented Oct 6, 2022

Added the API:

SendBatch(ctx context.Context, batch []hrpc.Call) (res []hrpc.RPCResult, allOK bool)

The res has results in the same order as the RPCs in batch. Each RPCResult contains a result and an error. If the error is non-nil then the RPC failed or was not attempted. If it's nil the batch succeeded. The allOK is false if any of the results contain an error.

closes #68, closes #117

@aaronbee aaronbee force-pushed the batch branch 2 times, most recently from 0a4d9e2 to 71f965d Compare October 7, 2022 20:31
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Patch coverage: 94.07% and project coverage change: +0.80 🎉

Comparison is base (09d0b0f) 69.54% compared to head (90be048) 70.35%.

❗ Current head 90be048 differs from pull request most recent head 023669f. Consider uploading reports for the commit 023669f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   69.54%   70.35%   +0.80%     
==========================================
  Files          27       27              
  Lines        3596     3714     +118     
==========================================
+ Hits         2501     2613     +112     
- Misses        979      983       +4     
- Partials      116      118       +2     
Impacted Files Coverage Δ
hrpc/call.go 70.00% <ø> (ø)
region/new.go 0.00% <0.00%> (ø)
region/client.go 82.95% <94.44%> (+0.17%) ⬆️
rpc.go 85.52% <94.69%> (+2.15%) ⬆️
region/multi.go 87.79% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@ciacono ciacono left a comment

Choose a reason for hiding this comment

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

Initial pass

batch.go Outdated Show resolved Hide resolved
batch.go Outdated Show resolved Hide resolved
region/multi.go Show resolved Hide resolved
rpc.go Outdated Show resolved Hide resolved
@aaronbee aaronbee force-pushed the batch branch 3 times, most recently from 08d5895 to 00358c1 Compare May 15, 2023 22:10
@aaronbee aaronbee force-pushed the batch branch 2 times, most recently from 49d2be8 to a9e6988 Compare June 12, 2023 22:47
@aaronbee
Copy link
Collaborator Author

Latest version changes the API of SendBatch and gets rid of the Batch type:

New API:

SendBatch(ctx context.Context, batch []hrpc.Call) (res []hrpc.RPCResult, err error)

The res has results in the same order as the RPCs in batch. Each RPCResult contains a result and an error. If the error is non-nil then the RPC failed or was not attempted. If it's nil the batch succeeded. The error return is just set to non-nil if any of the RPCResults contain an error.

@aaronbee
Copy link
Collaborator Author

Progress update: The new API feels better than my first attempt. This change is still missing tests.

@aaronbee aaronbee changed the title Add Batch API for sending a batch of mutate requests Add SendBatch API Jun 12, 2023
@aaronbee aaronbee force-pushed the batch branch 2 times, most recently from 32f41a9 to 6ffc0ac Compare June 23, 2023 23:59
@aaronbee
Copy link
Collaborator Author

Added unit tests for SendBatch. I think there are just a couple small changes I want to make. The change is ready for review.

@dethi
Copy link
Collaborator

dethi commented Jul 18, 2023

Closes #68 #117

@aaronbee aaronbee force-pushed the batch branch 3 times, most recently from 35f258d to 90be048 Compare July 18, 2023 20:51
rpc.go Show resolved Hide resolved
rpc.go Outdated Show resolved Hide resolved
Change the rpcs chan from holding hrpc.Call to []hrpc.Call. This is
required for batch support.
Allows passing a batch of RPCs to the region client, which will be
added to the in progress multi and then sent to HBASE.

A note here is that it's possible for a multi to grow beyond the size
of the configured size. This feels like the right way to go because
the user of the Batch API has specifically requested the rpcs be
batched together, so splitting the batch across multiple multi's
breaks that bit of atomicity.
Removes a type assert in SendBatch.
Copy link
Collaborator

@dethi dethi left a comment

Choose a reason for hiding this comment

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

Overall, LGTM Aaron

return res, allOK
}

rpcByClient, ok := c.findClients(ctx, batch, res)
Copy link
Collaborator

@dethi dethi Jul 27, 2023

Choose a reason for hiding this comment

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

Could be interesting to add an histogram to track how many regions are "targeted" by batch of RPC. To be added in following PR maybe.

@aaronbee aaronbee merged commit 3acbbb4 into tsuna:master Jul 27, 2023
2 checks passed
@aaronbee aaronbee deleted the batch branch July 27, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gohbase support batch gets now? if yes, how to use pls Manually build multi-actions?
4 participants