Break apart container configuration build and push step #252
Conversation
|
|
||
| return containerConfigurationBlobDescriptor; | ||
| return new BlobAndDigest(containerConfigurationBlob, digestOutputStream.toBlobDescriptor()); |
There was a problem hiding this comment.
This could probably avoid creating a new tuple class BlobAndDigest by just returning the Blob here and generating the Digest in the push step.
There was a problem hiding this comment.
Sounds good, I wasn't sure if the Digest generation should be kept in the build step.
| new PushContainerConfigurationStep( | ||
| buildConfiguration, | ||
| authenticatePushFuture, | ||
| buildContainerConfigurationFutureFuture.get(), |
There was a problem hiding this comment.
This call to get would be executed before anything is run, which would cause race conditions and blocking. Any get for a Future should be called in the call method of the step that uses it and should be wrapped in a NonBlockingFutures.get for busy-wait safety (it makes sure that the get does not block).
| ListeningExecutorService listeningExecutorService) { | ||
| this.buildConfiguration = buildConfiguration; | ||
| this.pushAuthorizationFuture = pushAuthorizationFuture; | ||
| this.blobFuturesFuture = blobFuturesFuture; |
There was a problem hiding this comment.
nit: *FutureFuture since it is just a future wrapping a since future
There was a problem hiding this comment.
Might also be clearer to name this like buildContainerConfigurationF... to make it explicit where this should be coming from.
| import java.util.concurrent.Callable; | ||
| import java.util.concurrent.ExecutionException; | ||
|
|
||
| class PushContainerConfigurationStep implements Callable<ListenableFuture<BlobDescriptor>> { |
| @@ -59,11 +60,11 @@ | |||
| @Override | |||
| public ListenableFuture<Blob> call() throws ExecutionException, InterruptedException { | |||
| // TODO: This might need to belong in BuildImageSteps. | |||
| List<ListenableFuture<?>> afterBaseImageLayerFuturesFutureDependencies = new ArrayList<>(); | |||
| afterBaseImageLayerFuturesFutureDependencies.addAll( | |||
| List<ListenableFuture<?>> afterBaseImageLayerFutureFutureDependencies = new ArrayList<>(); | |||
There was a problem hiding this comment.
Oh this one is actually a future of futures (it refers to being after pullBaseImageLayerFuturesFuture)
| @@ -78,7 +80,8 @@ private BlobDescriptor afterBlobFuturesFuture() | |||
|
|
|||
| CountingDigestOutputStream digestOutputStream = | |||
| new CountingDigestOutputStream(ByteStreams.nullOutputStream()); | |||
| Blob blob = NonBlockingFutures.get(NonBlockingFutures.get(blobFuturesFuture)); | |||
| Blob blob = | |||
There was a problem hiding this comment.
Maybe containerConfigurationBlob for specificity?
| /** Depends on {@code blobFuturesFuture} and {@code pushAuthorizationFuture}. */ | ||
| @Override | ||
| public ListenableFuture<BlobDescriptor> call() throws ExecutionException, InterruptedException { | ||
| List<ListenableFuture<?>> blobFuturesFutureDependencies = new ArrayList<>(); |
There was a problem hiding this comment.
This is a after...FutureFuture too. afterBuildContainerConfigurationFutureFutureDependencies would be the exact name (meaning the dependencies for the afterBuildContainerConfigurationFutureFuture method), but it is quite long.
There was a problem hiding this comment.
How about afterBuildConfigFutureFuture so it isn't so long? I think when variable names get too long they become just as hard to understand as short names.
There was a problem hiding this comment.
Hmm yeah we try to avoid abbreviations, so not sure what the best practice here should be. @loosebazooka
There was a problem hiding this comment.
Genereally we want to avoid abbreviations.
There was a problem hiding this comment.
Went with afterBuildConfigurationFutureFuture.
| } | ||
|
|
||
| /** Depends on {@code blobFuturesFuture.get()} and {@code pushAuthorizationFuture}. */ | ||
| private BlobDescriptor afterBlobFuturesFuture() |
Separating build and push steps so we can build and export to Docker daemon.