damccorm commented on code in PR #41167:
URL: https://github.com/apache/beam/pull/41167#discussion_r4527287636
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerCommand.java:
##########
@@ -88,28 +88,53 @@ private DockerCommand(String dockerExecutable, Duration
commandTimeout, Duration
public String runImage(String imageTag, List<String> dockerOpts,
List<String> args)
throws IOException, TimeoutException, InterruptedException {
checkArgument(!imageTag.isEmpty(), "Docker image tag required");
- // Pull the image from docker repo. This will be no-op if the image
already exists.
- try {
- runShortCommand(
-
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
- pullTimeout);
- } catch (IOException | TimeoutException | InterruptedException e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Unable to pull docker image {}", imageTag, e);
- } else {
- LOG.warn("Unable to pull docker image {}", imageTag, e);
+ if (!isImagePresentLocally(imageTag)) {
+ // Pull the image from docker repo if it does not already exist locally.
+ try {
+ runShortCommand(
+
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
+ pullTimeout);
+ } catch (IOException | TimeoutException | InterruptedException e) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Unable to pull docker image {}", imageTag, e);
+ } else {
+ LOG.warn("Unable to pull docker image {}", imageTag, e);
+ }
}
}
- // TODO: Validate args?
- return runShortCommand(
+ List<String> runCommand =
ImmutableList.<String>builder()
.add(dockerExecutable)
.add("run")
.add("-d")
.addAll(dockerOpts)
.add(imageTag)
.addAll(args)
- .build());
+ .build();
+ try {
+ return runShortCommand(runCommand);
+ } catch (TimeoutException e) {
+ LOG.warn("Timed out starting docker container for image {}, retrying
once", imageTag, e);
+ return runShortCommand(runCommand);
+ }
+ }
+
+ private boolean isImagePresentLocally(String imageTag) {
+ try {
+ runShortCommand(
+ ImmutableList.<String>builder()
+ .add(dockerExecutable)
+ .add("image")
+ .add("inspect")
+ .add(imageTag)
+ .build());
+ return true;
Review Comment:
Done — updated `isImagePresentLocally` to declare `throws
InterruptedException` and let it propagate directly.
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerCommand.java:
##########
@@ -88,28 +88,53 @@ private DockerCommand(String dockerExecutable, Duration
commandTimeout, Duration
public String runImage(String imageTag, List<String> dockerOpts,
List<String> args)
throws IOException, TimeoutException, InterruptedException {
checkArgument(!imageTag.isEmpty(), "Docker image tag required");
- // Pull the image from docker repo. This will be no-op if the image
already exists.
- try {
- runShortCommand(
-
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
- pullTimeout);
- } catch (IOException | TimeoutException | InterruptedException e) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Unable to pull docker image {}", imageTag, e);
- } else {
- LOG.warn("Unable to pull docker image {}", imageTag, e);
+ if (!isImagePresentLocally(imageTag)) {
+ // Pull the image from docker repo if it does not already exist locally.
+ try {
+ runShortCommand(
+
ImmutableList.<String>builder().add(dockerExecutable).add("pull").add(imageTag).build(),
+ pullTimeout);
+ } catch (IOException | TimeoutException | InterruptedException e) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Unable to pull docker image {}", imageTag, e);
+ } else {
+ LOG.warn("Unable to pull docker image {}", imageTag, e);
+ }
}
}
- // TODO: Validate args?
- return runShortCommand(
+ List<String> runCommand =
ImmutableList.<String>builder()
.add(dockerExecutable)
.add("run")
.add("-d")
.addAll(dockerOpts)
.add(imageTag)
.addAll(args)
- .build());
+ .build();
+ try {
+ return runShortCommand(runCommand);
+ } catch (TimeoutException e) {
+ LOG.warn("Timed out starting docker container for image {}, retrying
once", imageTag, e);
+ return runShortCommand(runCommand);
+ }
+ }
+
+ private boolean isImagePresentLocally(String imageTag) {
+ try {
+ runShortCommand(
+ ImmutableList.<String>builder()
+ .add(dockerExecutable)
+ .add("image")
Review Comment:
Done — added `-f {{.Id}}` to `docker image inspect`.
--
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]
