[BEAM-7195] BQ BatchLoads doesn't always create new tables #14238
Conversation
reuvenlax
changed the title
|
I tried to fix these issues in pr/14238 |
|
@chamikaramj - Could you review this PR? |
|
Yeah, looking. |
| abstract Boolean isFirstPane(); | ||
| } | ||
|
|
||
| static class ResultCoder extends AtomicCoder<WriteTables.Result> { |
There was a problem hiding this comment.
Did you mean to redefine Result and ResultCoder classes here ? They are Already defined in the WritePartition class and definitions seems to be identical. Can we move these to new files and re-use ?
There was a problem hiding this comment.
they are a bit different - the one in WritePartition contains a list of filenames, while this one contains a singular filename.
| @@ -65,19 +65,24 @@ | |||
| import java.util.regex.Matcher; | |||
| import java.util.regex.Pattern; | |||
| import java.util.stream.Collectors; | |||
| import java.util.stream.StreamSupport; | |||
There was a problem hiding this comment.
Is it possible to add a new unit test that would break for the previous implementation (due to pane numbers being incorrect) but would pass for the new implementation ?
There was a problem hiding this comment.
possibly - trying to think how to best make that trigger.
reuvenlax
force-pushed
the
fix_bq_triggering
branch
from
9843c78
to
d9d5087
Compare
|
Java PreCommit failure is probably related: https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/16793/ |
|
Any plans to move forward with this? Could it be causing our nightly snapshot failures as well? I added some info to BEAM-7195. |
|
Checking in on the status here, is this ready to merge? |
I do not think so. I fixed the merge conflict but there are bunch of tests errors and they are probably relevant (related to: testWriteTable - https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/18382/) |
|
Looking - this PR got a bit stale.
…On Thu, Jul 22, 2021 at 2:06 PM Ahmet Altay ***@***.***> wrote:
Checking in on the status here, is this ready to merge?
I do not think so. I fixed the merge conflict but there are bunch of tests
errors and they are probably relevant (related to: testWriteTable -
https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/18382/)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#14238 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAYJVLVP5J5WJW6UXSXQMLTZCB43ANCNFSM4ZHIUEYA>
.
|
reuvenlax
force-pushed
the
fix_bq_triggering
branch
from
6b11149
to
e6a8c1e
Compare
|
rebased and fixed broken test |
|
There is a check style error: INSTANCE' must match pattern '^[a-z][a-zA-Z0-9]*_?$'. [StaticVariableName] |
|
CheckStyle fixed |
|
Is it ready now? It is passing tests. |
|
I believe.so |
|
LGTM. I will merge. |
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.