tvalentyn commented on code in PR #39288:
URL: https://github.com/apache/beam/pull/39288#discussion_r3587991426
##########
sdks/python/container/boot.go:
##########
@@ -580,10 +581,13 @@ func logRuntimeDependencies(ctx context.Context,
bufLogger *tools.BufferedLogger
}
bufLogger.Printf(ctx, "Dependencies in %s:", phase)
args = []string{"-m", "pip", "freeze", "--all"}
- if err := execx.ExecuteEnvWithIO(nil, os.Stdin, bufLogger, bufLogger,
pythonVersion, args...); err != nil {
+
+ var stdout bytes.Buffer
+ if err := execx.ExecuteEnvWithIO(nil, os.Stdin, &stdout, bufLogger,
pythonVersion, args...); err != nil {
Review Comment:
Thanks, @shunping ! This PR allows for additional flexibility to retain logs
and output them in a better form.
My main worry is with the complexity a developer should reason about to get
things right:
- remembering the behavior or buffered logger that splits lines
- knowing how to bypass line splitting
- knowing how to properly flush and print accumulated logs.
Do you think we could simplify this with better helpers? Perhaps with
something like:
// executeAndBatchLogs runs a command and batches its output into a single
// DEBUG log message on success or ERROR log message on failure.
func executeAndBatchLogs(ctx context.Context, env map[string]string,
bufLogger *tools.BufferedLogger, prog string, args ...string) error {
...
}
// executeAndStreamLogs runs a command and streams both its standard output
and
// standard error directly to the provided BufferedLogger. This allows the
BufferedLogger
// to automatically trigger its periodic flush interval for potentially
long-running commands.
func executeAndStreamLogs(ctx context.Context, env map[string]string,
bufLogger *tools.BufferedLogger, prog string, args ...string) error {
...
}
WDYT?
##########
sdks/python/container/boot.go:
##########
@@ -580,10 +581,13 @@ func logRuntimeDependencies(ctx context.Context,
bufLogger *tools.BufferedLogger
}
bufLogger.Printf(ctx, "Dependencies in %s:", phase)
Review Comment:
Could we please append `Dependencies in` to the log itself?
Maybe we should also prepend it with "Runtime dependencies" ?
Context: this way it would be easier to search in the cloud console
"dependencies" and seeing the line that filters the dependenices + content.
We should do the same for submission deps as well.
Fine to do in a separate PR if you feel this is out of scope or file an
issue if you'd rather not do this rn.
--
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]
