-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Python BQ] Follow up of #28592: Allow setting a fixed number of Storage API streams #28618
Conversation
Run Python_Xlang_Gcp_Dataflow PostCommit |
Run Python_Xlang_Gcp_Direct PostCommit |
Codecov Report
@@ Coverage Diff @@
## master #28618 +/- ##
==========================================
+ Coverage 72.20% 72.22% +0.01%
==========================================
Files 684 684
Lines 101131 101133 +2
==========================================
+ Hits 73023 73042 +19
+ Misses 26529 26512 -17
Partials 1579 1579
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
retest this please |
Run Python_Xlang_Gcp_Direct PostCommit |
Run Python_Xlang_Gcp_Dataflow PostCommit |
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
@@ -176,6 +176,13 @@ public void validate() { | |||
!Strings.isNullOrEmpty(this.getErrorHandling().getOutput()), | |||
invalidConfigMessage + "Output must not be empty if error handling specified."); | |||
} | |||
|
|||
if (this.getAutoSharding() != null && this.getAutoSharding()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be a check on the main BigQuery transform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar check does exist in the top-level BigQueryIO transform:
Lines 3226 to 3230 in c82b329
if (getAutoSharding()) { | |
if (method == Method.STORAGE_WRITE_API && getStorageApiNumStreams(bqOptions) > 0) { | |
LOG.warn( | |
"Both numStorageWriteApiStreams and auto-sharding options are set. Will default to auto-sharding." | |
+ " To set a fixed number of streams, do not enable auto-sharding."); |
...along with many other warnings we put in place just because we don't want to push breaking changes. In the case of a Beam 3 type of event, we would make all these checks throw errors. With that in mind, I added a checkArgument
when introducing the numStreams option.
Also: Please don't merge with failing GitHub statuses. Kick them to green. And the coverage complaint seems legitimate. |
@kennknowles some FhirIO tests are very flaky (maybe perma-failing) on Github workflow #28635 |
Had to fix some lint checks in #28592 which was reverted in #28613