Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on May 8, 2026. It is now read-only.

chore: add filter size check on read row request#166

Merged
igorbernstein2 merged 3 commits into
googleapis:mastergoogleapis/java-bigtable:masterfrom
rahulKQL:filterSizerahulKQL/java-bigtable:filterSizeCopy head branch name to clipboard
Jan 27, 2020
Merged

chore: add filter size check on read row request#166
igorbernstein2 merged 3 commits into
googleapis:mastergoogleapis/java-bigtable:masterfrom
rahulKQL:filterSizerahulKQL/java-bigtable:filterSizeCopy head branch name to clipboard

Conversation

@rahulKQL

Copy link
Copy Markdown
Contributor

Currently, bigtable could only serve read row requests with the filter size of 20MB. Adding this check to prevent failure.

Currently, bigtable could only serve read row request with filter size of 20MB only. So adding this check to stop server request.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 23, 2020
@rahulKQL rahulKQL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 23, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 23, 2020
kolea2
kolea2 previously requested changes Jan 23, 2020
*/
public Query filter(Filters.Filter filter) {
builder.setFilter(filter.toProto());
Preconditions.checkNotNull(filter, "filter can't be null");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we add a test please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Added test case for null check. I haven't added any test case for the filter size. I hope that should be fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apologies for being unclear, if you could add a test case for filter size as well that would be awesome :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I intentionally didn't add test case for that. Reason being the filter size needs to be more than 20MB, I thought that'd be a little large.

I would add unit test right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Understood - LGTM. @igorbernstein2 any other thoughts?

@codecov

codecov Bot commented Jan 23, 2020

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@bc0fb86). Click here to learn what that means.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #166   +/-   ##
=========================================
  Coverage          ?   81.89%           
  Complexity        ?      971           
=========================================
  Files             ?       99           
  Lines             ?     6025           
  Branches          ?      331           
=========================================
  Hits              ?     4934           
  Misses            ?      912           
  Partials          ?      179
Impacted Files Coverage Δ Complexity Δ
...om/google/cloud/bigtable/data/v2/models/Query.java 67.88% <80%> (ø) 26 <1> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc0fb86...091fcd8. Read the comment docs.

@kolea2 kolea2 dismissed their stale review January 23, 2020 19:08

stale review

@igorbernstein2 igorbernstein2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@igorbernstein2 igorbernstein2 merged commit c5baee0 into googleapis:master Jan 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.