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

[BEAM-6076] Fetch only required fields for BigQuery Table #7066

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

lgajowy
Copy link
Contributor

@lgajowy lgajowy commented Nov 16, 2018

Before this change, all table fields were fetched which caused a
NullPointerException in StandardTableDefinition.java class -
the specific reason is that StreamingBuffer's "estimatedRows"
field is sometimes null. This change makes the client to skip fetching
this field so that it doesn't use the erroneous code.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

Before this change, all table fields were fetched which caused a
NullPointerException in StandardTableDefinition.java class -
the specific reason is that StreamingBuffer's "estimatedRows"
field is sometimes null. This change makes the client to skip fetching
This field so that it doesn't use the erroneous code.
@lgajowy
Copy link
Contributor Author

lgajowy commented Nov 16, 2018

I think this will settle the problem. As far as I can tell, the problematic field was estimated rows field that sometimes seems to be null (hence NPE when calling longValue() on it). The proposed change skips invoking that code so that we do not spot the bug anymore (this "problematic" field was not needed either way).

Other than that such query will be a little bit more resource-efficient since the response body is smaller now (not fetching all fields of Table JSON).

@apilloud could you take a look?

@lgajowy
Copy link
Contributor Author

lgajowy commented Nov 16, 2018

For reference: googleapis/google-cloud-java#3982

@lgajowy
Copy link
Contributor Author

lgajowy commented Nov 16, 2018

Run JavaPortabilityApi PreCommit

Copy link
Member

@apilloud apilloud left a comment

Choose a reason for hiding this comment

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

LGTM

@lgajowy lgajowy merged commit ef3b052 into apache:master Nov 19, 2018
@lgajowy
Copy link
Contributor Author

lgajowy commented Nov 19, 2018

Despite the fact that this issue is not happening anymore (we might be lucky) I'm merging this. Fetching all fields is still not needed so it's a good improvement. Thanks!

@lgajowy lgajowy deleted the fix-nexmark-flakiness branch November 19, 2018 16:16
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.

2 participants