aritra24 commented on code in PR #50964: URL: https://github.com/apache/airflow/pull/50964#discussion_r2102807722
########## go-sdk/celery/cmd/root.go: ########## @@ -0,0 +1,158 @@ +// 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 cmd + +import ( + "context" + "fmt" + "os" + "path" + "strings" + + cc "github.com/ivanpirog/coloredcobra" + "github.com/spf13/cobra" + "github.com/spf13/pflag" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +var cfgFile string + +// rootCmd represents the base command when called without any subcommands +var rootCmd = &cobra.Command{ + Use: "airflow-go-sdk", + Short: "A brief description of your application", + Long: `A longer description that spans multiple lines and likely contains +examples and usage of using your application. For example: + +Cobra is a CLI library for Go that empowers applications. +This application is a tool to generate the needed files +to quickly create a Cobra application. Review Comment: nit: This needs to be updated? ########## go-sdk/celery/app.go: ########## @@ -0,0 +1,106 @@ +// 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 celery + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "os" + "os/signal" + + celery "github.com/marselester/gopher-celery" + celeryredis "github.com/marselester/gopher-celery/goredis" + "github.com/redis/go-redis/v9" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/api" + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +func Run(ctx context.Context, config Config) error { + worker, ok := ctx.Value(sdkcontext.WorkerContextKey).(worker.Worker) + if !ok { + slog.ErrorContext(ctx, "Worker missing in context") + return fmt.Errorf("worker value missing in context") + } + + worker, err := worker.WithServer(viper.GetString("execution-api-url")) + if err != nil { + slog.ErrorContext(ctx, "Error setting ExecutionAPI sxerver for worker", "err", err) + return err + } + + // Store the updated worker in context + ctx = context.WithValue(ctx, sdkcontext.WorkerContextKey, worker) + + ctx, stop := signal.NotifyContext(ctx, os.Interrupt) + defer stop() + + c := redis.NewClient(&redis.Options{ + Addr: config.BrokerAddr, + }) + defer func() { + if err := c.Close(); err != nil { + slog.ErrorContext(ctx, "failed to close Redis client", "err", err) + } + }() + + if _, err := c.Ping(ctx).Result(); err != nil { + slog.ErrorContext(ctx, "Redis connection failed", "err", err) + return err + } + broker := celeryredis.NewBroker( + celeryredis.WithClient(c), + ) + + if len(config.Queues) == 0 { + config.Queues = append(config.Queues, "default") + } Review Comment: nit: If we're definining queues with a default of "default" the only case you end up here is if you explicitly set queues to empty in the config, in which case should we rather error out? instead of silently adding default? ########## go-sdk/celery/app.go: ########## @@ -0,0 +1,106 @@ +// 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 celery + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "os" + "os/signal" + + celery "github.com/marselester/gopher-celery" + celeryredis "github.com/marselester/gopher-celery/goredis" + "github.com/redis/go-redis/v9" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/api" + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +func Run(ctx context.Context, config Config) error { + worker, ok := ctx.Value(sdkcontext.WorkerContextKey).(worker.Worker) + if !ok { + slog.ErrorContext(ctx, "Worker missing in context") + return fmt.Errorf("worker value missing in context") Review Comment: Same on the other logs ########## go-sdk/celery/cmd/root.go: ########## @@ -0,0 +1,158 @@ +// 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 cmd + +import ( + "context" + "fmt" + "os" + "path" + "strings" + + cc "github.com/ivanpirog/coloredcobra" + "github.com/spf13/cobra" + "github.com/spf13/pflag" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +var cfgFile string + +// rootCmd represents the base command when called without any subcommands +var rootCmd = &cobra.Command{ + Use: "airflow-go-sdk", + Short: "A brief description of your application", + Long: `A longer description that spans multiple lines and likely contains +examples and usage of using your application. For example: + +Cobra is a CLI library for Go that empowers applications. +This application is a tool to generate the needed files +to quickly create a Cobra application. + `, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return initializeConfig(cmd) + }, +} + +// Execute adds all child commands to the root command and sets flags appropriately. +// This is called by main.main(). It only needs to happen once to the rootCmd. +func Execute(worker worker.Worker) { + cc.Init(&cc.Config{ + RootCmd: rootCmd, + Headings: cc.Bold, + Commands: cc.Yellow + cc.Bold, + Example: cc.Italic, + ExecName: cc.HiMagenta + cc.Bold, + Flags: cc.Green, + FlagsDataType: cc.Italic + cc.White, + }) + // Store the worker in the context so we can pull it out later + ctx := context.WithValue(context.Background(), sdkcontext.WorkerContextKey, worker) + err := rootCmd.ExecuteContext(ctx) + if err != nil { + os.Exit(1) + } +} + +func init() { + rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", + "config file (default is $HOME/airflow/go-sdk.yaml)") + rootCmd.AddCommand(runCmd) +} + +func setupViper() error { + if cfgFile != "" { + // Use config file from the flag. + viper.SetConfigFile(cfgFile) + } else { + airflowHome := os.Getenv("AIRFLOW_HOME") + if airflowHome == "" { + home, err := os.UserHomeDir() + cobra.CheckErr(err) + airflowHome = path.Join(home, "airflow") + } + + // Search config in home directory with name ".go-sdk" (without extension). + viper.AddConfigPath(airflowHome) + viper.SetConfigType("yaml") + viper.SetConfigName("go-sdk.yaml") + } + + viper.SetOptions(viper.ExperimentalBindStruct()) + + // Attempt to read the config file, gracefully ignoring errors + // caused by a config file not being found. Return an error + // if we cannot parse the config file. + if err := viper.ReadInConfig(); err != nil { + // It's okay if there isn't a config file + if _, ok := err.(viper.ConfigFileNotFoundError); !ok { + return err + } + } + + // Make the prefix be AIRFLOW__ -- viper adds an extra `_` automatically + viper.SetEnvPrefix("AIRFLOW_") + // Set the key replacer to replace "__" with "." + replacer := strings.NewReplacer(".", "__", "-", "_") + viper.SetEnvKeyReplacer(replacer) + + viper.AutomaticEnv() // read in environment variables that match + return nil +} + +// initConfig reads in config file and ENV variables if set. +func initializeConfig(cmd *cobra.Command) error { + if err := setupViper(); err != nil { + return err + } + + // Set the key replacer to replace "__" with "." + replacer := strings.NewReplacer(".", "__", "-", "_") Review Comment: nit: The order seems to be off? Replacer's doc says old, new so it would replace . with __ and - with _? ########## go-sdk/celery/cmd/root.go: ########## @@ -0,0 +1,158 @@ +// 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 cmd + +import ( + "context" + "fmt" + "os" + "path" + "strings" + + cc "github.com/ivanpirog/coloredcobra" + "github.com/spf13/cobra" + "github.com/spf13/pflag" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +var cfgFile string + +// rootCmd represents the base command when called without any subcommands +var rootCmd = &cobra.Command{ + Use: "airflow-go-sdk", + Short: "A brief description of your application", + Long: `A longer description that spans multiple lines and likely contains +examples and usage of using your application. For example: + +Cobra is a CLI library for Go that empowers applications. +This application is a tool to generate the needed files +to quickly create a Cobra application. + `, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return initializeConfig(cmd) + }, +} + +// Execute adds all child commands to the root command and sets flags appropriately. +// This is called by main.main(). It only needs to happen once to the rootCmd. +func Execute(worker worker.Worker) { + cc.Init(&cc.Config{ + RootCmd: rootCmd, + Headings: cc.Bold, + Commands: cc.Yellow + cc.Bold, + Example: cc.Italic, + ExecName: cc.HiMagenta + cc.Bold, + Flags: cc.Green, + FlagsDataType: cc.Italic + cc.White, + }) + // Store the worker in the context so we can pull it out later + ctx := context.WithValue(context.Background(), sdkcontext.WorkerContextKey, worker) + err := rootCmd.ExecuteContext(ctx) + if err != nil { + os.Exit(1) + } +} + +func init() { + rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", + "config file (default is $HOME/airflow/go-sdk.yaml)") + rootCmd.AddCommand(runCmd) +} + +func setupViper() error { + if cfgFile != "" { + // Use config file from the flag. + viper.SetConfigFile(cfgFile) + } else { + airflowHome := os.Getenv("AIRFLOW_HOME") + if airflowHome == "" { + home, err := os.UserHomeDir() + cobra.CheckErr(err) + airflowHome = path.Join(home, "airflow") + } + + // Search config in home directory with name ".go-sdk" (without extension). + viper.AddConfigPath(airflowHome) + viper.SetConfigType("yaml") + viper.SetConfigName("go-sdk.yaml") + } + + viper.SetOptions(viper.ExperimentalBindStruct()) + + // Attempt to read the config file, gracefully ignoring errors + // caused by a config file not being found. Return an error + // if we cannot parse the config file. + if err := viper.ReadInConfig(); err != nil { + // It's okay if there isn't a config file + if _, ok := err.(viper.ConfigFileNotFoundError); !ok { + return err + } + } + + // Make the prefix be AIRFLOW__ -- viper adds an extra `_` automatically + viper.SetEnvPrefix("AIRFLOW_") + // Set the key replacer to replace "__" with "." + replacer := strings.NewReplacer(".", "__", "-", "_") + viper.SetEnvKeyReplacer(replacer) + + viper.AutomaticEnv() // read in environment variables that match + return nil +} + +// initConfig reads in config file and ENV variables if set. +func initializeConfig(cmd *cobra.Command) error { + if err := setupViper(); err != nil { + return err + } + + // Set the key replacer to replace "__" with "." + replacer := strings.NewReplacer(".", "__", "-", "_") Review Comment: Also can we reuse the slice between both functions? So we can isolate the changes. ########## go-sdk/celery/app.go: ########## @@ -0,0 +1,106 @@ +// 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 celery + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "os" + "os/signal" + + celery "github.com/marselester/gopher-celery" + celeryredis "github.com/marselester/gopher-celery/goredis" + "github.com/redis/go-redis/v9" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/api" + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +func Run(ctx context.Context, config Config) error { + worker, ok := ctx.Value(sdkcontext.WorkerContextKey).(worker.Worker) + if !ok { + slog.ErrorContext(ctx, "Worker missing in context") + return fmt.Errorf("worker value missing in context") Review Comment: nit: I think we shouldn't log here, since we're pushing it up and would be logging again when Run fails, I think it's better to return this and wrap the error for higher up. ########## go-sdk/celery/app.go: ########## @@ -0,0 +1,106 @@ +// 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 celery + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "os" + "os/signal" + + celery "github.com/marselester/gopher-celery" + celeryredis "github.com/marselester/gopher-celery/goredis" + "github.com/redis/go-redis/v9" + "github.com/spf13/viper" + + "github.com/apache/airflow/go-sdk/pkg/api" + "github.com/apache/airflow/go-sdk/pkg/sdkcontext" + "github.com/apache/airflow/go-sdk/worker" +) + +func Run(ctx context.Context, config Config) error { + worker, ok := ctx.Value(sdkcontext.WorkerContextKey).(worker.Worker) + if !ok { + slog.ErrorContext(ctx, "Worker missing in context") + return fmt.Errorf("worker value missing in context") + } + + worker, err := worker.WithServer(viper.GetString("execution-api-url")) + if err != nil { + slog.ErrorContext(ctx, "Error setting ExecutionAPI sxerver for worker", "err", err) + return err + } + + // Store the updated worker in context + ctx = context.WithValue(ctx, sdkcontext.WorkerContextKey, worker) + + ctx, stop := signal.NotifyContext(ctx, os.Interrupt) + defer stop() + + c := redis.NewClient(&redis.Options{ + Addr: config.BrokerAddr, + }) + defer func() { + if err := c.Close(); err != nil { + slog.ErrorContext(ctx, "failed to close Redis client", "err", err) + } + }() + + if _, err := c.Ping(ctx).Result(); err != nil { + slog.ErrorContext(ctx, "Redis connection failed", "err", err) + return err + } + broker := celeryredis.NewBroker( + celeryredis.WithClient(c), + ) + + if len(config.Queues) == 0 { + config.Queues = append(config.Queues, "default") + } + broker.Observe(config.Queues) + + app := celery.NewApp( + celery.WithBroker(broker), + ) + + for _, queue := range config.Queues { + app.Register( + "execute_workload", + queue, + func(ctx context.Context, p *celery.TaskParam) error { + p.NameArgs("payload") + payload := p.MustString("payload") + + var workload api.ExecuteTaskWorkload + if err := json.Unmarshal([]byte(payload), &workload); err != nil { + return err + } + return worker.ExecuteTaskWorkload(ctx, workload) + }, + ) + } + + slog.Info("waiting for tasks", "queues", config.Queues) + err = app.Run(ctx) + if err != nil { + slog.Info("program stopped", "error", err) Review Comment: Should this be an error log? -- 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]
