lostluck commented on code in PR #35369:
URL: https://github.com/apache/beam/pull/35369#discussion_r2162118152
##########
sdks/go/pkg/beam/io/pubsubio/pubsubio.go:
##########
@@ -52,32 +52,50 @@ func init() {
// ReadOptions represents options for reading from PubSub.
type ReadOptions struct {
+ Topic string
Subscription string
IDAttribute string
TimestampAttribute string
WithAttributes bool
}
// Read reads an unbounded number of PubSubMessages from the given
-// pubsub topic. It produces an unbounded PCollecton<*PubSubMessage>,
+// pubsub topic or subscription. It produces an unbounded
PCollecton<*PubSubMessage>,
// if WithAttributes is set, or an unbounded PCollection<[]byte>.
-func Read(s beam.Scope, project, topic string, opts *ReadOptions)
beam.PCollection {
+func Read(s beam.Scope, project string, opts *ReadOptions) beam.PCollection {
Review Comment:
Since we're making a breaking change, lets go all in. Make it `opts
ReadOptions` which avoids the value being nil (and saving us the check), while
also requiring the value to be configured.
##########
sdks/go/pkg/beam/io/pubsubio/pubsubio.go:
##########
@@ -52,32 +52,50 @@ func init() {
// ReadOptions represents options for reading from PubSub.
type ReadOptions struct {
+ Topic string
Subscription string
IDAttribute string
TimestampAttribute string
WithAttributes bool
}
// Read reads an unbounded number of PubSubMessages from the given
-// pubsub topic. It produces an unbounded PCollecton<*PubSubMessage>,
+// pubsub topic or subscription. It produces an unbounded
PCollecton<*PubSubMessage>,
// if WithAttributes is set, or an unbounded PCollection<[]byte>.
Review Comment:
```suggestion
// if WithAttributes is set, or an unbounded PCollection<[]byte>.
//
// The topic or subscription is required and must be set with ReadOptions.
```
##########
sdks/go/pkg/beam/io/pubsubio/pubsubio.go:
##########
@@ -52,32 +52,50 @@ func init() {
// ReadOptions represents options for reading from PubSub.
type ReadOptions struct {
+ Topic string
Subscription string
Review Comment:
```suggestion
Topic string // Topic sets the topic to be read from. A
new subscription will be generated for the job. Mutually exclusive with setting
a Subscription.
Subscription string // Subscription sets the name of an existing
subscription to read from. Mutually exclusive with setting a subscription.
```
##########
sdks/go/pkg/beam/io/pubsubio/pubsubio.go:
##########
@@ -52,32 +52,50 @@ func init() {
// ReadOptions represents options for reading from PubSub.
type ReadOptions struct {
+ Topic string
Subscription string
IDAttribute string
TimestampAttribute string
WithAttributes bool
}
// Read reads an unbounded number of PubSubMessages from the given
-// pubsub topic. It produces an unbounded PCollecton<*PubSubMessage>,
+// pubsub topic or subscription. It produces an unbounded
PCollecton<*PubSubMessage>,
// if WithAttributes is set, or an unbounded PCollection<[]byte>.
-func Read(s beam.Scope, project, topic string, opts *ReadOptions)
beam.PCollection {
Review Comment:
The main issue with this PR is that it introduced a compile time breaking
change to the API, which is considered poor form in Go. We've largely tried to
avoid doing that for the SDK's life, to make upgrading easier.
But this is an awkward gap, and any hard check here would be a breaking
change anyway, and it's better that it's at compile time, than at runtime.
I request that you add a message to root/CHANGES.md describing the breaking
change at least (as well as my other suggestion on the Read docs.)
--
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.
To unsubscribe, e-mail: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
