[jira] [Work logged] (BEAM-8018) Detect unexported fields in unregistered types for better error messages
[ https://issues.apache.org/jira/browse/BEAM-8018?focusedWorklogId=299507=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-299507 ] ASF GitHub Bot logged work on BEAM-8018: Author: ASF GitHub Bot Created on: 22/Aug/19 15:55 Start Date: 22/Aug/19 15:55 Worklog Time Spent: 10m Work Description: lostluck commented on pull request #9387: [BEAM-8018] Detect unexported fields earlier URL: https://github.com/apache/beam/pull/9387 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 299507) Time Spent: 50m (was: 40m) > Detect unexported fields in unregistered types for better error messages > > > Key: BEAM-8018 > URL: https://issues.apache.org/jira/browse/BEAM-8018 > Project: Beam > Issue Type: Improvement > Components: sdk-go >Reporter: Robert Burke >Assignee: Robert Burke >Priority: Minor > Time Spent: 50m > Remaining Estimate: 0h > > Unregistered types with unexported fields cause pipeline execution time > panics rather than construction time panics/errors, which would be preferable. > Improved error messages encouraging users to register their types already > exists, but isn't currently triggered in this instance, so it's a matter of > returning an error in this case. > eg. for using time.Time as a value in a DoFn... > panic: > > reflect.StructOf: StructOf does not allow unexported fields [recovered] > panic: reflect.StructOf: StructOf does not allow unexported fields > goroutine 195 [running]: > panic(0x7e0a060, 0x84beb70) > third_party/go/gc/src/runtime/panic.go:567 +0x2da fp=0xc000dc1178 > sp=0xc000dc10c0 pc=0xee24daa > testing.tRunner.func1(0xc000d6ec00) > third_party/go/gc/src/testing/testing.go:830 +0x388 fp=0xc000dc11f8 > sp=0xc000dc1178 pc=0xfafda58 > runtime.call32(0x0, 0x833e1e0, 0xc000caaab0, 0x80008) > third_party/go/gc/src/runtime/asm_amd64.s:519 +0x3b fp=0xc000dc1228 > sp=0xc000dc11f8 pc=0xee53acb > panic(0x7e0a060, 0x84beb70) > third_party/go/gc/src/runtime/panic.go:522 +0x1b5 fp=0xc000dc12e0 > sp=0xc000dc1228 pc=0xee24c85 > reflect.runtimeStructField(0xc000e267e0, 0x4, 0xc000e267e4, 0x4, 0x8541880, > 0x7e0a060, 0x0, 0x0, 0x0, 0xc000e26990, ...) > third_party/go/gc/src/reflect/type.go:2765 +0x1c2 fp=0xc000dc1348 > sp=0xc000dc12e0 pc=0xee80ea2 > reflect.StructOf(0xc0006dd040, 0x3, 0x4, 0x0, 0x0) > third_party/go/gc/src/reflect/type.go:2371 +0x21f6 fp=0xc000dc1b28 > sp=0xc000dc1348 pc=0xee7f816 > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48800|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48800], > 0xc000dc1e68, 0xee3c4f2, 0xc000e26920, 0x0) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:558 > +0xa6c fp=0xc000dc1df8 sp=0xc000dc1b28 pc=0xf53a7ac > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48780|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48780], > 0x0, 0x0, 0x0, 0x1) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:533 > +0x471 fp=0xc000dc20c8 sp=0xc000dc1df8 pc=0xf53a1b1 > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48680|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48680], > 0xc000369bf0, 0xc000b24a40, 0x3e, 0xc000961728) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:542 > +0x6c5 fp=0xc000dc2398 sp=0xc000dc20c8 pc=0xf53a405 > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48600|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48600], > 0xc0001af340, 0x1, 0x1, 0x2) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:583 > +0x12fc fp=0xc000dc2668 sp=0xc000dc2398 pc=0xf53b03c >
[jira] [Work logged] (BEAM-8018) Detect unexported fields in unregistered types for better error messages
[
https://issues.apache.org/jira/browse/BEAM-8018?focusedWorklogId=299052=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-299052
]
ASF GitHub Bot logged work on BEAM-8018:
Author: ASF GitHub Bot
Created on: 21/Aug/19 22:53
Start Date: 21/Aug/19 22:53
Worklog Time Spent: 10m
Work Description: youngoli commented on pull request #9387: [BEAM-8018]
Detect unexported fields earlier
URL: https://github.com/apache/beam/pull/9387#discussion_r316434083
##
File path: sdks/go/pkg/beam/core/runtime/graphx/serialize_test.go
##
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package graphx
+
+import (
+ "reflect"
+ "strings"
+ "testing"
+
+ "github.com/apache/beam/sdks/go/pkg/beam/core/runtime"
+ v1 "github.com/apache/beam/sdks/go/pkg/beam/core/runtime/graphx/v1"
+)
+
+func TestEncodeType(t *testing.T) {
+ t.Run("NoUnexportedFields", func(t *testing.T) {
+ type MyAwesomeType struct {
+ ExportedField string
+ }
+ rt := reflect.TypeOf((*MyAwesomeType)(nil)).Elem()
+
+ pbT, err := encodeType(rt)
+ if err != nil {
+ t.Fatalf("got error = %v, want nil", err)
+ }
+ if got, want := pbT.Kind, v1.Type_STRUCT; got != want {
+ t.Fatalf("got pbT.Kin == %v, want %v", got, want)
+ }
+ })
+ t.Run("UnregisteredWithUnexportedField", func(t *testing.T) {
+ type MyProblematicType struct {
+ unexportedField string
+ }
+ rt := reflect.TypeOf((*MyProblematicType)(nil)).Elem()
+ pbT, err := encodeType(rt)
+ if err == nil {
+ t.Fatalf("got type = %v, nil, want unexported field
error", pbT)
+ }
+ if !strings.Contains(err.Error(), "type has unexported field:
unexportedField") {
+ t.Errorf("expected error about unexported field, got
%q", err.Error())
+ }
+ })
+ t.Run("RegisteredWithUnexportedField", func(t *testing.T) {
+ type MyRegisteredType struct {
+ unexportedField string
+ }
+ rt := reflect.TypeOf((*MyRegisteredType)(nil)).Elem()
+ runtime.RegisterType(rt)
+ pbT, err := encodeType(rt)
+ if err != nil {
+ t.Fatalf("got error = %v, want nil", err)
+ }
+ if got, want := pbT.Kind, v1.Type_EXTERNAL; got != want {
+ t.Fatalf("got pbT.Kin == %v, want %v", got, want)
Review comment:
Same typo as above.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 299052)
Time Spent: 40m (was: 0.5h)
> Detect unexported fields in unregistered types for better error messages
>
>
> Key: BEAM-8018
> URL: https://issues.apache.org/jira/browse/BEAM-8018
> Project: Beam
> Issue Type: Improvement
> Components: sdk-go
>Reporter: Robert Burke
>Assignee: Robert Burke
>Priority: Minor
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Unregistered types with unexported fields cause pipeline execution time
> panics rather than construction time panics/errors, which would be preferable.
> Improved error messages encouraging users to register their types already
> exists, but isn't currently triggered in this instance, so it's a matter of
> returning an error in this case.
> eg. for using time.Time as a value in a DoFn...
> panic:
>
>
[jira] [Work logged] (BEAM-8018) Detect unexported fields in unregistered types for better error messages
[
https://issues.apache.org/jira/browse/BEAM-8018?focusedWorklogId=299051=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-299051
]
ASF GitHub Bot logged work on BEAM-8018:
Author: ASF GitHub Bot
Created on: 21/Aug/19 22:53
Start Date: 21/Aug/19 22:53
Worklog Time Spent: 10m
Work Description: youngoli commented on pull request #9387: [BEAM-8018]
Detect unexported fields earlier
URL: https://github.com/apache/beam/pull/9387#discussion_r316431985
##
File path: sdks/go/pkg/beam/core/runtime/graphx/serialize_test.go
##
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package graphx
+
+import (
+ "reflect"
+ "strings"
+ "testing"
+
+ "github.com/apache/beam/sdks/go/pkg/beam/core/runtime"
+ v1 "github.com/apache/beam/sdks/go/pkg/beam/core/runtime/graphx/v1"
+)
+
+func TestEncodeType(t *testing.T) {
+ t.Run("NoUnexportedFields", func(t *testing.T) {
+ type MyAwesomeType struct {
+ ExportedField string
+ }
+ rt := reflect.TypeOf((*MyAwesomeType)(nil)).Elem()
+
+ pbT, err := encodeType(rt)
+ if err != nil {
+ t.Fatalf("got error = %v, want nil", err)
+ }
+ if got, want := pbT.Kind, v1.Type_STRUCT; got != want {
+ t.Fatalf("got pbT.Kin == %v, want %v", got, want)
Review comment:
Nit: Typo.
```suggestion
t.Fatalf("got pbT.Kind == %v, want %v", got, want)
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 299051)
Time Spent: 0.5h (was: 20m)
> Detect unexported fields in unregistered types for better error messages
>
>
> Key: BEAM-8018
> URL: https://issues.apache.org/jira/browse/BEAM-8018
> Project: Beam
> Issue Type: Improvement
> Components: sdk-go
>Reporter: Robert Burke
>Assignee: Robert Burke
>Priority: Minor
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Unregistered types with unexported fields cause pipeline execution time
> panics rather than construction time panics/errors, which would be preferable.
> Improved error messages encouraging users to register their types already
> exists, but isn't currently triggered in this instance, so it's a matter of
> returning an error in this case.
> eg. for using time.Time as a value in a DoFn...
> panic:
>
> reflect.StructOf: StructOf does not allow unexported fields [recovered]
> panic: reflect.StructOf: StructOf does not allow unexported fields
> goroutine 195 [running]:
> panic(0x7e0a060, 0x84beb70)
> third_party/go/gc/src/runtime/panic.go:567 +0x2da fp=0xc000dc1178
> sp=0xc000dc10c0 pc=0xee24daa
> testing.tRunner.func1(0xc000d6ec00)
> third_party/go/gc/src/testing/testing.go:830 +0x388 fp=0xc000dc11f8
> sp=0xc000dc1178 pc=0xfafda58
> runtime.call32(0x0, 0x833e1e0, 0xc000caaab0, 0x80008)
> third_party/go/gc/src/runtime/asm_amd64.s:519 +0x3b fp=0xc000dc1228
> sp=0xc000dc11f8 pc=0xee53acb
> panic(0x7e0a060, 0x84beb70)
> third_party/go/gc/src/runtime/panic.go:522 +0x1b5 fp=0xc000dc12e0
> sp=0xc000dc1228 pc=0xee24c85
> reflect.runtimeStructField(0xc000e267e0, 0x4, 0xc000e267e4, 0x4, 0x8541880,
> 0x7e0a060, 0x0, 0x0, 0x0, 0xc000e26990, ...)
> third_party/go/gc/src/reflect/type.go:2765 +0x1c2 fp=0xc000dc1348
> sp=0xc000dc12e0 pc=0xee80ea2
> reflect.StructOf(0xc0006dd040, 0x3, 0x4, 0x0, 0x0)
> third_party/go/gc/src/reflect/type.go:2371 +0x21f6 fp=0xc000dc1b28
> sp=0xc000dc1348 pc=0xee7f816
>
[jira] [Work logged] (BEAM-8018) Detect unexported fields in unregistered types for better error messages
[ https://issues.apache.org/jira/browse/BEAM-8018?focusedWorklogId=298262=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-298262 ] ASF GitHub Bot logged work on BEAM-8018: Author: ASF GitHub Bot Created on: 20/Aug/19 22:23 Start Date: 20/Aug/19 22:23 Worklog Time Spent: 10m Work Description: lostluck commented on issue #9387: [BEAM-8018] Detect unexported fields earlier URL: https://github.com/apache/beam/pull/9387#issuecomment-523218379 R: @youngoli This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 298262) Time Spent: 20m (was: 10m) > Detect unexported fields in unregistered types for better error messages > > > Key: BEAM-8018 > URL: https://issues.apache.org/jira/browse/BEAM-8018 > Project: Beam > Issue Type: Improvement > Components: sdk-go >Reporter: Robert Burke >Assignee: Robert Burke >Priority: Minor > Time Spent: 20m > Remaining Estimate: 0h > > Unregistered types with unexported fields cause pipeline execution time > panics rather than construction time panics/errors, which would be preferable. > Improved error messages encouraging users to register their types already > exists, but isn't currently triggered in this instance, so it's a matter of > returning an error in this case. > eg. for using time.Time as a value in a DoFn... > panic: > > reflect.StructOf: StructOf does not allow unexported fields [recovered] > panic: reflect.StructOf: StructOf does not allow unexported fields > goroutine 195 [running]: > panic(0x7e0a060, 0x84beb70) > third_party/go/gc/src/runtime/panic.go:567 +0x2da fp=0xc000dc1178 > sp=0xc000dc10c0 pc=0xee24daa > testing.tRunner.func1(0xc000d6ec00) > third_party/go/gc/src/testing/testing.go:830 +0x388 fp=0xc000dc11f8 > sp=0xc000dc1178 pc=0xfafda58 > runtime.call32(0x0, 0x833e1e0, 0xc000caaab0, 0x80008) > third_party/go/gc/src/runtime/asm_amd64.s:519 +0x3b fp=0xc000dc1228 > sp=0xc000dc11f8 pc=0xee53acb > panic(0x7e0a060, 0x84beb70) > third_party/go/gc/src/runtime/panic.go:522 +0x1b5 fp=0xc000dc12e0 > sp=0xc000dc1228 pc=0xee24c85 > reflect.runtimeStructField(0xc000e267e0, 0x4, 0xc000e267e4, 0x4, 0x8541880, > 0x7e0a060, 0x0, 0x0, 0x0, 0xc000e26990, ...) > third_party/go/gc/src/reflect/type.go:2765 +0x1c2 fp=0xc000dc1348 > sp=0xc000dc12e0 pc=0xee80ea2 > reflect.StructOf(0xc0006dd040, 0x3, 0x4, 0x0, 0x0) > third_party/go/gc/src/reflect/type.go:2371 +0x21f6 fp=0xc000dc1b28 > sp=0xc000dc1348 pc=0xee7f816 > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48800|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48800], > 0xc000dc1e68, 0xee3c4f2, 0xc000e26920, 0x0) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:558 > +0xa6c fp=0xc000dc1df8 sp=0xc000dc1b28 pc=0xf53a7ac > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48780|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48780], > 0x0, 0x0, 0x0, 0x1) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:533 > +0x471 fp=0xc000dc20c8 sp=0xc000dc1df8 pc=0xf53a1b1 > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48680|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48680], > 0xc000369bf0, 0xc000b24a40, 0x3e, 0xc000961728) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:542 > +0x6c5 fp=0xc000dc2398 sp=0xc000dc20c8 pc=0xf53a405 > [google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48600|https://cs.corp.google.com/piper///depot/google3/third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/graphx.decodeType(0xc000c48600], > 0xc0001af340, 0x1, 0x1, 0x2) > > third_party/golang/apache_beam/pkg/beam/core/runtime/graphx/serialize.go:583 > +0x12fc fp=0xc000dc2668 sp=0xc000dc2398 pc=0xf53b03c >
[jira] [Work logged] (BEAM-8018) Detect unexported fields in unregistered types for better error messages
[ https://issues.apache.org/jira/browse/BEAM-8018?focusedWorklogId=298260=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-298260 ] ASF GitHub Bot logged work on BEAM-8018: Author: ASF GitHub Bot Created on: 20/Aug/19 22:23 Start Date: 20/Aug/19 22:23 Worklog Time Spent: 10m Work Description: lostluck commented on pull request #9387: [BEAM-8018] Detect unexported fields earlier URL: https://github.com/apache/beam/pull/9387 Values of unregistered types in the Beam Go SDK are synthetically constructed with the reflect package. However, it's not possible to synthetically construct types with unexported fields with reflect.StructOf. This can be worked around by registering the given type, which avoids the expensive synthetic construction and this failure mode. The failure manifests at pipeline execution time which is worse. This PR moves that failure to pipeline construction time, where it presently manifests as a panic (until BEAM-8017 is resolved) with instructions how to register the type. Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily: - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`). - [ ] 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](https://www.apache.org/licenses/icla.pdf). Post-Commit Tests Status (on master branch) Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark --- | --- | --- | --- | --- | --- | --- | --- Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/) Python | [