[GitHub] [beam] damccorm commented on a change in pull request #16957: [BEAM-13829] Expose status API from Go SDK Harness

[GitBox <gi...@apache.org>]

damccorm commented on a change in pull request #16957:
URL: https://github.com/apache/beam/pull/16957#discussion_r816958547



##########
File path: sdks/go/pkg/beam/core/runtime/harness/harness.go
##########
@@ -36,14 +36,27 @@ import (
 	"google.golang.org/grpc"
 )
 
+// StatusAddress is a type of status endpoint address as an optional argument to harness.Main().
+type StatusAddress string
+
 // TODO(herohde) 2/8/2017: for now, assume we stage a full binary (not a plugin).
 
 // Main is the main entrypoint for the Go harness. It runs at "runtime" -- not
 // "pipeline-construction time" -- on each worker. It is a FnAPI client and
 // ultimately responsible for correctly executing user code.
-func Main(ctx context.Context, loggingEndpoint, controlEndpoint string) error {
+func Main(ctx context.Context, loggingEndpoint, controlEndpoint string, options ...interface{}) error {
 	hooks.DeserializeHooksFromOptions(ctx)
 
+	statusEndpoint := ""
+	for _, option := range options {
+		switch option := option.(type) {
+		case StatusAddress:
+			statusEndpoint = string(option)
+		default:
+			return errors.Errorf("unkown type %T, value %v in error call", option, option)

Review comment:
       You're printing option twice here - did you mean for the first to be option.(type)?

##########
File path: sdks/go.mod
##########
@@ -23,33 +23,39 @@ module github.com/apache/beam/sdks/v2
 go 1.16
 
 require (
-	cloud.google.com/go/bigquery v1.17.0
-	cloud.google.com/go/datastore v1.5.0
-	cloud.google.com/go/pubsub v1.11.0-beta.schemas
-	cloud.google.com/go/storage v1.15.0
+	cloud.google.com/go/bigquery v1.28.0
+	cloud.google.com/go/compute v1.5.0 // indirect
+	cloud.google.com/go/datastore v1.6.0
+	cloud.google.com/go/iam v0.2.0 // indirect
+	cloud.google.com/go/pubsub v1.18.0
+	cloud.google.com/go/storage v1.21.0
+	github.com/bketelsen/crypt v0.0.4 // indirect

Review comment:
       Were these version changes intentional/do you have a need for them? I'm not opposed to doing it if you don't need it, just trying to understand how we normally handle this - in general, do we have a strategy on when to update 3rd party dependencies?

##########
File path: sdks/go/pkg/beam/core/runtime/harness/harness.go
##########
@@ -98,6 +111,18 @@ func Main(ctx context.Context, loggingEndpoint, controlEndpoint string) error {
 		log.Debugf(ctx, "control response channel closed")
 	}()
 
+	// if the runner supports worker status api then expose SDK harness status
+	if statusEndpoint != "" {
+		statusHandler, err := newWorkerStatusHandler(ctx, statusEndpoint)
+		if err != nil {
+			log.Error(ctx, err)

Review comment:
       If we return an error response, is it worth trying to handle requests or should we stick the remaining logic in an else? I'd imagine that its just going to fail since we failed to set up our connection.




-- 
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-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org