jrmccluskey commented on code in PR #27158: URL: https://github.com/apache/beam/pull/27158#discussion_r1235651479
########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment + +import ( + "fmt" + "os" + "strings" +) + +// Variable defines an environment variable via a string type alias. +// Variable's string value assigns the system environment variable key. +type Variable string + +// Default a value to the system environment. Review Comment: This comment isn't clear for what the method is doing ########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment + +import ( + "fmt" + "os" + "strings" +) + +// Variable defines an environment variable via a string type alias. +// Variable's string value assigns the system environment variable key. +type Variable string + +// Default a value to the system environment. +func (v Variable) Default(value string) error { + if v.Missing() { + return os.Setenv((string)(v), value) + } + return nil +} + +// Missing reports whether the system environment variable is an empty string. +func (v Variable) Missing() bool { + return v.Value() == "" +} + +// Key returns the system environment variable key. +func (v Variable) Key() string { + return (string)(v) +} + +// Value returns the system environment variable value. +func (v Variable) Value() string { + return os.Getenv((string)(v)) +} + +// KeyValue returns a concatenated string of the system environment variable's +// <key>=<value>. +func (v Variable) KeyValue() string { + return fmt.Sprintf("%s=%s", (string)(v), v.Value()) +} + +// Missing reports as an error listing all Variable among vars that are +// not assigned in the system environment. +func Missing(vars ...Variable) error { + var missing []string + for _, v := range vars { + if v.Missing() { + missing = append(missing, v.KeyValue()) + } + } + if len(missing) > 0 { + return fmt.Errorf("variables empty but expected from environtment: %s", strings.Join(missing, "; ")) + } + return nil +} + +// Map converts a slice of Variable into a map. +// Its usage is for logging purposes. +func Map(vars ...Variable) map[string]interface{} { + result := map[string]interface{}{} Review Comment: Use`make(map[string]any)` to instantiate the map (https://gobyexample.com/maps) `any` is now equivalent to `interface{}` and is generally preferable now. ########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment + +import ( + "fmt" + "os" + "strings" +) + +// Variable defines an environment variable via a string type alias. +// Variable's string value assigns the system environment variable key. +type Variable string + +// Default a value to the system environment. +func (v Variable) Default(value string) error { + if v.Missing() { + return os.Setenv((string)(v), value) + } + return nil +} + +// Missing reports whether the system environment variable is an empty string. +func (v Variable) Missing() bool { + return v.Value() == "" +} + +// Key returns the system environment variable key. +func (v Variable) Key() string { + return (string)(v) +} + +// Value returns the system environment variable value. +func (v Variable) Value() string { + return os.Getenv((string)(v)) +} Review Comment: Nit: The Key-Value naming here isn't inaccurate, but it feels somewhat strained since the `Variable` type itself only encapsulates the string name of the environment variable. ########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment + +import ( + "fmt" + "os" + "strings" +) + +// Variable defines an environment variable via a string type alias. +// Variable's string value assigns the system environment variable key. +type Variable string + +// Default a value to the system environment. +func (v Variable) Default(value string) error { + if v.Missing() { + return os.Setenv((string)(v), value) + } + return nil +} + +// Missing reports whether the system environment variable is an empty string. +func (v Variable) Missing() bool { + return v.Value() == "" +} + +// Key returns the system environment variable key. +func (v Variable) Key() string { + return (string)(v) +} + +// Value returns the system environment variable value. +func (v Variable) Value() string { + return os.Getenv((string)(v)) +} + +// KeyValue returns a concatenated string of the system environment variable's +// <key>=<value>. +func (v Variable) KeyValue() string { + return fmt.Sprintf("%s=%s", (string)(v), v.Value()) +} + +// Missing reports as an error listing all Variable among vars that are Review Comment: ```suggestion // Missing returns an error listing all Variable among vars that are ``` ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/echo/main.go: ########## @@ -0,0 +1,124 @@ +// 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. + +// Echo executes the service to fulfill requests that mock a quota-aware API +// endpoint. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/echo" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" + + address = fmt.Sprintf(":%s", port.Value()) + logger = logging.New( + context.Background(), + "github.com/apache/beam/.test-infra/pipelines/src/main/go/cmd/api_overuse_study/echo", + logging.LevelVariable) + cacheQuota cache.Decrementer + + required = []environment.Variable{ + port, + cache.Host, + } + + env = environment.Map(required...) Review Comment: This is unnecessary, for the variadic you can just pass `port` and `cache.Host` to `environment.Map`. https://gobyexample.com/variadic-functions ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/refresher/main.go: ########## @@ -0,0 +1,135 @@ +// 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. + +// Refresher executes a backend service whose single job is to refresh a quota. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "strconv" + "time" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/redis/go-redis/v9" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + required = []environment.Variable{ + cache.Host, + cache.QuotaId, + cache.QuotaSize, + cache.QuotaRefreshInterval, + } + + size uint64 + interval time.Duration + + logger = logging.New( + context.Background(), + "github.com/apache/beam/.test-infra/pipelines/src/main/go/cmd/api_overuse_study/refresher", + logging.LevelVariable) + cacheRefresher cache.Refresher + subscriber cache.Subscriber + + env = logging.Any("env", environment.Map(required...)) Review Comment: Same variadic note ########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment + +import ( + "fmt" + "os" + "strings" +) + +// Variable defines an environment variable via a string type alias. +// Variable's string value assigns the system environment variable key. +type Variable string + +// Default a value to the system environment. Review Comment: The method name doesn't seem to match the implementation either, since the function only sets the default IFF the variable doesn't already have a value. ########## .test-infra/pipelines/src/main/go/internal/cache/interface.go: ########## @@ -0,0 +1,52 @@ +// 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 cache Review Comment: Is the plan to generalize the functionality you're needing here for different frameworks in future experiments? The interfaces here are only implemented by the redis cache right now, so they seem excessive unless you're going to have variations ########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment Review Comment: This package should get unit tests. Just remember to reset any environment variables you're using between tests with helpers (I'm looking for an example or a reference for how to do this cleanly and go but can't find one off the top, I'll try to circle back) ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/echo/main.go: ########## @@ -0,0 +1,124 @@ +// 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. + +// Echo executes the service to fulfill requests that mock a quota-aware API +// endpoint. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/echo" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" + + address = fmt.Sprintf(":%s", port.Value()) + logger = logging.New( + context.Background(), + "github.com/apache/beam/.test-infra/pipelines/src/main/go/cmd/api_overuse_study/echo", + logging.LevelVariable) + cacheQuota cache.Decrementer + + required = []environment.Variable{ + port, + cache.Host, + } + + env = environment.Map(required...) +) + +func init() { + ctx := context.Background() + + if err := environment.Missing(required...); err != nil { + logger.Fatal(ctx, err, logging.Any("env", env)) + } + + if err := vars(ctx); err != nil { + logger.Fatal(ctx, err, logging.Any("env", env)) + } + +} + +func vars(ctx context.Context) error { Review Comment: This function name is pretty misleading since it's actually making a redis client. You're also starting to get closer to naming collisions with the environment variable stuff ########## .test-infra/pipelines/src/main/go/internal/environment/variable.go: ########## @@ -0,0 +1,82 @@ +// 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 environment provides helpers for interacting with environment +// variables. +package environment + +import ( + "fmt" + "os" + "strings" +) + +// Variable defines an environment variable via a string type alias. +// Variable's string value assigns the system environment variable key. +type Variable string + +// Default a value to the system environment. +func (v Variable) Default(value string) error { + if v.Missing() { + return os.Setenv((string)(v), value) Review Comment: All of the `(string)(v)` casts should be able to be written as `string(v)` ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota/main.go: ########## @@ -0,0 +1,144 @@ +// 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. + +// Quota executes the service to fulfill requests that manage the API quota. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/k8s" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/quota" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" Review Comment: May be worth having an extra utils package were you can stash shared environment variables like this and any common functions between the packages for ease of use and improvement ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota/main.go: ########## @@ -0,0 +1,144 @@ +// 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. + +// Quota executes the service to fulfill requests that manage the API quota. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/k8s" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/quota" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" + refresherImage environment.Variable = "REFRESHER_IMAGE" + namespace environment.Variable = "NAMESPACE" + + spec = "a.ServiceSpec{ + RefresherServiceSpec: "a.RefresherServiceSpec{ + ContainerName: "refresher", + Image: refresherImage.Value(), + Environment: []environment.Variable{ + namespace, + cache.Host, + logging.LevelVariable, + }, + }, + } + + logger = logging.New( + context.Background(), + "github.com/apache/beam/.test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota", + logging.LevelVariable) + + required = []environment.Variable{ + port, + cache.Host, + namespace, + refresherImage, + } + + env = environment.Map(required...) Review Comment: same note on the varaidic ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota/main.go: ########## @@ -0,0 +1,144 @@ +// 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. + +// Quota executes the service to fulfill requests that manage the API quota. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/k8s" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/quota" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" + refresherImage environment.Variable = "REFRESHER_IMAGE" + namespace environment.Variable = "NAMESPACE" + + spec = "a.ServiceSpec{ + RefresherServiceSpec: "a.RefresherServiceSpec{ + ContainerName: "refresher", + Image: refresherImage.Value(), + Environment: []environment.Variable{ + namespace, + cache.Host, + logging.LevelVariable, + }, + }, + } + + logger = logging.New( + context.Background(), + "github.com/apache/beam/.test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota", + logging.LevelVariable) + + required = []environment.Variable{ + port, + cache.Host, + namespace, + refresherImage, + } + + env = environment.Map(required...) + address = fmt.Sprintf(":%s", port.Value()) +) + +func init() { + ctx := context.Background() + if err := environment.Missing(required...); err != nil { + logger.Fatal(ctx, err, logging.Any("env", env)) + } + + if err := vars(ctx); err != nil { + logger.Fatal(ctx, err, logging.Any("env", env)) + } +} + +func vars(ctx context.Context) error { + redisClient := redis.NewClient(&redis.Options{ + Addr: cache.Host.Value(), + }) + + cacheClient := (*cache.RedisCache)(redisClient) + + spec.Cache = cacheClient + spec.Publisher = cacheClient + + k8sClient, err := k8s.NewDefaultClient() + if err != nil { + return err + } + + ns := k8sClient.Namespace(namespace.Value()) + spec.JobsClient = k8sClient.Jobs(ns) + + if err := redisClient.Ping(ctx).Err(); err != nil { + return err + } + + logger.Info(ctx, "pinged cache host ok", logging.String("host", cache.Host.Value())) + + return nil +} Review Comment: Same note about the function name from the echo package ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota/main.go: ########## @@ -0,0 +1,144 @@ +// 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. + +// Quota executes the service to fulfill requests that manage the API quota. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/k8s" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/quota" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" + refresherImage environment.Variable = "REFRESHER_IMAGE" + namespace environment.Variable = "NAMESPACE" + + spec = "a.ServiceSpec{ + RefresherServiceSpec: "a.RefresherServiceSpec{ + ContainerName: "refresher", + Image: refresherImage.Value(), + Environment: []environment.Variable{ + namespace, + cache.Host, + logging.LevelVariable, + }, + }, + } + + logger = logging.New( + context.Background(), + "github.com/apache/beam/.test-infra/pipelines/src/main/go/cmd/api_overuse_study/quota", + logging.LevelVariable) + + required = []environment.Variable{ + port, + cache.Host, + namespace, + refresherImage, + } + + env = environment.Map(required...) + address = fmt.Sprintf(":%s", port.Value()) +) + +func init() { + ctx := context.Background() + if err := environment.Missing(required...); err != nil { + logger.Fatal(ctx, err, logging.Any("env", env)) + } + + if err := vars(ctx); err != nil { + logger.Fatal(ctx, err, logging.Any("env", env)) + } +} + +func vars(ctx context.Context) error { + redisClient := redis.NewClient(&redis.Options{ + Addr: cache.Host.Value(), + }) + + cacheClient := (*cache.RedisCache)(redisClient) + + spec.Cache = cacheClient + spec.Publisher = cacheClient + + k8sClient, err := k8s.NewDefaultClient() + if err != nil { + return err + } + + ns := k8sClient.Namespace(namespace.Value()) + spec.JobsClient = k8sClient.Jobs(ns) + + if err := redisClient.Ping(ctx).Err(); err != nil { + return err + } + + logger.Info(ctx, "pinged cache host ok", logging.String("host", cache.Host.Value())) Review Comment: Move the liveness check for the redis client above the kubernetes client work ########## .test-infra/pipelines/src/main/go/cmd/api_overuse_study/echo/main.go: ########## @@ -0,0 +1,124 @@ +// 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. + +// Echo executes the service to fulfill requests that mock a quota-aware API +// endpoint. +// To execute, simply run as a normal go executable. The application relies on +// environment variables for ease and compatibility with conventional cloud +// deployments such as a Kubernetes deployment specification. +// The executable reports missing required environment variables and serves +// as its own self-documenting usage. +// +// See logging.LevelVariable for details on how to change the log level. +package main + +import ( + "context" + "fmt" + "net" + + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/echo" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/pipelines/src/main/go/internal/logging" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" +) + +var ( + port environment.Variable = "PORT" + + address = fmt.Sprintf(":%s", port.Value()) Review Comment: Why not take this as a flag instead of an environment variable? The docstring for the package reads like you're spinning this up separately anyway. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
