-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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-4150] Remove fallback case for coder not specified within RemoteGrpcPort. #10755
Conversation
…emoteGrpcPort. Update the JRH in Dataflow to specify the coder id on the RemoteGrpcPort. All runners now specify the coder id so we can remove the fallback case.
Tested JRH change internally within Google as well. |
Run Python PreCommit |
Run Go Postcommit |
@@ -64,6 +64,13 @@ func UnmarshalPlan(desc *fnpb.ProcessBundleDescriptor) (*Plan, error) { | |||
} | |||
|
|||
u := &DataSource{UID: b.idgen.New()} | |||
u.Coder, err = b.coders.Coder(cid) // Expected to be windowed coder |
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.
Please run gofmt on your code. Go uses Tabs for indentation, not spaces.
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.
LGTM modulo aforementioned go formatting issues.
@lostluck have we considered checking gofmt as part of go precommit, similar to spotless java/groovy and the python formatting that's being added?
Run Python2_PVR_Flink PreCommit |
Run Java PreCommit |
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.
LGTM for Go
Tthey always have one.
…On Tue, Feb 4, 2020 at 11:04 AM Robert Burke ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In sdks/go/pkg/beam/core/runtime/exec/translate.go
<#10755 (comment)>:
> @@ -64,6 +64,13 @@ func UnmarshalPlan(desc *fnpb.ProcessBundleDescriptor) (*Plan, error) {
}
u := &DataSource{UID: b.idgen.New()}
+ u.Coder, err = b.coders.Coder(cid) // Expected to be windowed coder
+ if err != nil {
+ return nil, err
+ }
+ if !coder.IsW(u.Coder) {
+ return nil, errors.Errorf("unwindowed coder %v on DataSource %v: %v", cid, id, u.Coder)
+ }
for key, pid := range transform.GetOutputs() {
Do GRPC datasources always only have a single output?
I note that we aren't using the pid, in the new code.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#10755?email_source=notifications&email_token=ACM4V3D4ENJGCCU7DCMAVXLRBG333A5CNFSM4KPKV7XKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUG4ZCI#pullrequestreview-353225865>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACM4V3AXN6VNEWJK3UURZZ3RBG333ANCNFSM4KPKV7XA>
.
|
Whoops. Dead comment I thought I deleted after answering my own question
after expanding how much code was revealed.
…On Tue, Feb 4, 2020, 1:20 PM Lukasz Cwik ***@***.***> wrote:
Tthey always have one.
On Tue, Feb 4, 2020 at 11:04 AM Robert Burke ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In sdks/go/pkg/beam/core/runtime/exec/translate.go
> <#10755 (comment)>:
>
> > @@ -64,6 +64,13 @@ func UnmarshalPlan(desc
*fnpb.ProcessBundleDescriptor) (*Plan, error) {
> }
>
> u := &DataSource{UID: b.idgen.New()}
> + u.Coder, err = b.coders.Coder(cid) // Expected to be windowed coder
> + if err != nil {
> + return nil, err
> + }
> + if !coder.IsW(u.Coder) {
> + return nil, errors.Errorf("unwindowed coder %v on DataSource %v: %v",
cid, id, u.Coder)
> + }
>
> for key, pid := range transform.GetOutputs() {
>
> Do GRPC datasources always only have a single output?
>
> I note that we aren't using the pid, in the new code.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <
#10755?email_source=notifications&email_token=ACM4V3D4ENJGCCU7DCMAVXLRBG333A5CNFSM4KPKV7XKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUG4ZCI#pullrequestreview-353225865
>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/ACM4V3AXN6VNEWJK3UURZZ3RBG333ANCNFSM4KPKV7XA
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10755?email_source=notifications&email_token=ADKDOFKM6KGP2G7D7HTUARLRBHLZ5A5CNFSM4KPKV7XKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKZHI4I#issuecomment-582120561>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKDOFIPMLRNDWOLYIXBSOLRBHLZ5ANCNFSM4KPKV7XA>
.
|
All runners now specify the coder id.
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-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.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.