gemini-code-assist[bot] commented on code in PR #38530:
URL: https://github.com/apache/beam/pull/38530#discussion_r3261449041
##########
sdks/go/pkg/beam/runners/prism/internal/environments.go:
##########
@@ -249,10 +252,34 @@ func dockerEnvironment(ctx context.Context, logger
*slog.Logger, dp *pipepb.Dock
}()
bgctx := context.Background()
- statusCh, errCh := cli.ContainerWait(bgctx, containerID,
container.WaitConditionNotRunning)
+
+ // Wait for either context cancellation or container to stop
+ done := make(chan struct{})
+ go func() {
+ result := cli.ContainerWait(bgctx, containerID,
dcli.ContainerWaitOptions{
+ Condition: container.WaitConditionNotRunning,
+ })
+ if result.Error != nil {
+ logger.Error("docker container wait error",
"error", result.Error)
+ } else {
+ logger.Info("docker container has self
terminated")
+
+ rc, err := cli.ContainerLogs(bgctx,
containerID, dcli.ContainerLogsOptions{Details: true, ShowStdout: true,
ShowStderr: true})
+ if err != nil {
+ logger.Error("docker container logs
error", "error", err)
+ } else {
+ defer rc.Close()
+ var buf bytes.Buffer
+ stdcopy.StdCopy(&buf, &buf, rc)
+ logger.Error("container self
terminated", "log", buf.String())
Review Comment:
This logging logic has a few issues:
1. **Misleading Level**: It logs at the `Error` level even if the container
terminated successfully (exit code 0).
2. **Redundant Logging**: It logs "self terminated" twice (here and on line
265).
3. **Incorrect Message**: If the container was killed by the runner (via
`ContainerKill` on line 298), the goroutine will still log that it "self
terminated", which is misleading.
Consider checking the container's exit status and only logging an error if
it's non-zero, and ensure the message accurately reflects whether the
termination was expected or forced.
##########
sdks/go/pkg/beam/runners/prism/internal/environments.go:
##########
@@ -170,7 +169,7 @@ func dockerEnvironment(ctx context.Context, logger
*slog.Logger, dp *pipepb.Dock
logger = logger.With("worker_id", wk.ID, "image",
dp.GetContainerImage())
// TODO consider preserving client?
- cli, err := dcli.NewClientWithOpts(dcli.FromEnv,
dcli.WithAPIVersionNegotiation())
+ cli, err := dcli.New(dcli.FromEnv)
Review Comment:
The change from `dcli.NewClientWithOpts(dcli.FromEnv,
dcli.WithAPIVersionNegotiation())` to `dcli.New(dcli.FromEnv)` removes
automatic API version negotiation. This can lead to compatibility issues if the
client version is newer than the Docker daemon's supported API version. It is
generally recommended to keep version negotiation enabled to ensure the client
can communicate with various daemon versions.
```suggestion
cli, err := dcli.NewClientWithOpts(dcli.FromEnv,
dcli.WithAPIVersionNegotiation())
```
--
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]
