lostluck commented on code in PR #16658:
URL: https://github.com/apache/beam/pull/16658#discussion_r1015977504
##########
sdks/python/container/boot.go:
##########
@@ -137,46 +145,49 @@ func main() {
options, err := provision.ProtoToJSON(info.GetPipelineOptions())
if err != nil {
- log.Fatalf("Failed to convert pipeline options: %v", err)
+ return fmt.Errorf("Failed to convert pipeline options: %v", err)
}
// (2) Retrieve and install the staged packages.
//
- // Guard from concurrent artifact retrieval and installation,
- // when called by child processes in a worker pool.
+ // No log.Fatalf() from here on, otherwise deferred cleanups will not
be called!
Review Comment:
But the part I was commenting on is before any defer calls are being made.
This comment is about the factoring for making the function more readable and
maintainable, improving the change being made, not reverting it.
Basically the change is good, but the implementation scope is unnecessarily
broad. which puts additional strain on a reader for tracking which scope
they're in, when instead this change can accomplish it's goal (ensuring the
defered cleanups and goroutines are shutdown in an orderly fashion), and
improve readability, via smaller, easier to understand functions composed with
purpose.
##########
sdks/python/container/boot.go:
##########
@@ -73,13 +74,20 @@ const (
)
func main() {
+ if err := mainError(); err != nil {
+ log.Print(err)
+ os.Exit(1)
+ }
+}
+
+func mainError() error {
Review Comment:
Ping.
##########
sdks/python/container/boot.go:
##########
@@ -73,13 +74,20 @@ const (
)
func main() {
+ if err := mainError(); err != nil {
+ log.Print(err)
+ os.Exit(1)
+ }
+}
+
+func mainError() error {
flag.Parse()
Review Comment:
This should still be in the actual main(), and not tucked into the function.
##########
sdks/python/container/boot.go:
##########
@@ -73,13 +74,20 @@ const (
)
func main() {
+ if err := mainError(); err != nil {
+ log.Print(err)
+ os.Exit(1)
Review Comment:
OK, that's fair. The question is now why not use *log.Fatal* here instead of
the manual print + Exit. We're already out of the function scope.
--
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
