Adds DefaultCacheStorage#write. #879

coollog · 2018-08-24T19:03:07Z

Part of #637

DefaultCacheStorage - storage engine implementation with just write implemented so far
DefaultCacheStorageFiles - resolves files used in the cache storage directory
DefaultCacheStorageWriter - the logic behind DefaultCacheStorage#write

…/jib into ncache-1-interfaces

…ry to CacheEntry/CacheWrite.

…rage

…inerTools/jib into ncache-3-defaultcachestorage

loosebazooka · 2018-08-31T19:03:28Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorage.java

+
+  private final DefaultCacheStorageWriter defaultCacheStorageWriter;
+
+  private DefaultCacheStorage(Path cacheDirectory) {


Would we want to pass in defaultCacheStorageWriter so we can mock this out? Perhaps in withDirectory?

loosebazooka · 2018-08-31T19:32:48Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorage.java

+  }
+
+  @Override
+  public CacheEntry write(CacheWrite cacheWrite) throws IOException {


I don't know where CacheWrite came from? Perhaps an earlier PR? But it's naming is confusing. I would expect it to be "CacheLayerMetadata" or something

Hmm, it used be called CacheWriteEntry and CacheEntry used to be CacheReadEntry. CacheWrite is intended to be the interface for cache writes and CacheLayerMetadata might give a sense of being the layer metadata read out from the cache. Perhaps it should be called CacheWriteData instead?

loosebazooka · 2018-08-31T19:39:04Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorageWriter.java

+   * @throws IOException if an I/O exception occurs
+   */
+  CacheEntry write(CacheWrite cacheWrite) throws IOException {
+    Path temporaryLayerFile = Files.createTempFile(null, null);


I would expect this write to happen in the cache directory? I feel like mv is not free in Windows (but renames are?)

TadCordle · 2018-09-05T20:20:40Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorage.java

+ *   ...
+ * }</pre>
+ *
+ * Layers entires are stored in their own directories under the {@code layers/} directory. Each


~~entires~~ entries

TadCordle · 2018-09-05T20:30:15Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorageFiles.java

+   * @return the selector file
+   */
+  Path getSelectorFile(DescriptorDigest selector) {
+    return getSelectorDirectory(selector);


Is getSelectorDirectory necessary, or can its logic just go here?

TadCordle · 2018-09-05T20:34:33Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorageWriter.java

+import java.util.zip.GZIPOutputStream;
+
+/** Writes to the default cache storage engine. */
+class DefaultCacheStorageWriter {


I think since this class only contains one write method that's used only in DefaultCacheStorage, we could move that logic there instead of adding this extra class. Although I'm not sure what the plan is for reusing this.

This is broken out so that it can be independently worked on in relation to the reading methods (listDigests, retrieve, select) as part of a DefaultCacheStorageReader. This will help keep the reading and writing logic separate, componentized, and independently-testable.

TadCordle · 2018-09-05T20:38:39Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorageWriter.java

+      Path layerFile = defaultCacheStorageFiles.getLayerFile(layerDigest, layerDiffId);
+      Files.createDirectories(layerFile.getParent());
+      try {
+        Files.move(temporaryLayerFile, layerFile);


Could moving this file while we have an output stream open on it cause problems? Can we close the output stream earlier?

The file is actually closed on line 62, which is definitely not clear here. I'll try to think of a way to make this more readable.

coollog · 2018-09-06T19:30:38Z

Incorporated new proposals to account for atomicity of writes from #878 (comment). This changes the write to only produce a layer directory at the intended location in full. This way, cache reads never see an incomplete layer directory.

loosebazooka · 2018-09-07T21:27:40Z

jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/FileOperations.java

@@ -52,5 +57,27 @@ public static void copy(ImmutableList<Path> sourceFiles, Path destDir) throws IO
    }
  }

+  /**
+   * Acquires an exclusive lock on the {@code file} and opens an {@link OutputStream} to write to


I think should make our intentions a little more clear here on why we're locking. They are an assertion rather than concurrency management feature?

jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/TemporaryDirectory.java

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorageWriter.java

loosebazooka

I think it'd be nice to see an actual analysis of the performance when we switch to the new caching.

coollog · 2018-09-10T20:17:08Z

Definitely. We'll do a side-by-side comparison with cache once ncache is working.

commit 411da83 Author: Q Chen <[email protected]> Date: Mon Sep 10 16:17:22 2018 -0400 Adds DefaultCacheStorage#write. (#879) commit 56d818a Author: Chanseok Oh <[email protected]> Date: Fri Sep 7 12:39:22 2018 -0400 Restart Docker for Mac to fix clock skew in VM (#951) commit 0b2e09a Author: Chanseok Oh <[email protected]> Date: Thu Sep 6 16:09:28 2018 -0400 Lower verbosity of insecure registry failover (#945)

loosebazooka · 2018-09-10T20:46:09Z

jib-core/src/main/java/com/google/cloud/tools/jib/ncache/DefaultCacheStorageWriter.java

+
+    try (CountingDigestOutputStream compressedDigestOutputStream =
+        new CountingDigestOutputStream(
+            new BufferedOutputStream(FileOperations.newLockingOutputStream(temporaryLayerFile)))) {


Sorry I missed this, but it doesn't seem necessary to use locking outputstream on temporary files. Shouldn't their uniqueness be provided by guava?

Oh yes, that's true. Will change this in #898

coollog added 13 commits August 22, 2018 12:08

Adds new cache interfaces under package ncache.

e8b107e

Merge branch 'master' into ncache-1-interfaces

9713ff9

Merge branch 'master' into ncache-1-interfaces

336e74d

Merge branch 'ncache-1-interfaces' into ncache-2-default-impls

d403cea

Adds default implementations for CacheWriteEntry and CacheReadEntry.

1a9bbd9

Merge branch 'master' into ncache-1-interfaces

18d42c4

Adds more explanation to javadoc.

5b9c820

Merge branch 'ncache-1-interfaces' of github.com:GoogleContainerTools…

e41d447

…/jib into ncache-1-interfaces

Merge branch 'ncache-1-interfaces' into ncache-2-default-impls

74b481d

Adds tests.

0fac8f6

Merge branch 'master' into ncache-2-default-impls

c7337c9

Revert

a475ea0

Adds DefaultCacheStorage#save.

8330b04

googlebot added the cla: yes label Aug 24, 2018

coollog mentioned this pull request Aug 24, 2018

Reorganize caching to be an interface to allow for different implementations. #637

Closed

14 tasks

coollog added the PR: Not Ready label Aug 28, 2018

coollog added 9 commits August 28, 2018 17:52

Fixes CacheStorage interface and renames CacheReadEntry/CacheWriteEnt…

6d76f57

…ry to CacheEntry/CacheWrite.

Changes save to write.

8db9bdf

Merge branch 'ncache-2-default-impls-2' into ncache-3-defaultcachesto…

261775e

…rage

Fixes confflicts.

c02f80c

Merge branch 'master' into ncache-3-defaultcachestorage

0e777bb

Fixing up selector.

d01dd45

Merge branch 'master' into ncache-3-defaultcachestorage

3dbc3d6

Merge branch 'ncache-3-defaultcachestorage' of github.com:GoogleConta…

25a1f17

…inerTools/jib into ncache-3-defaultcachestorage

Fixes test.

ea2d426

coollog removed the PR: Not Ready label Aug 29, 2018

coollog requested a review from a team August 29, 2018 15:42

coollog changed the title ~~Adds DefaultCacheStorage#save.~~ Adds DefaultCacheStorage#write. Aug 29, 2018

coollog mentioned this pull request Aug 29, 2018

Adds DefaultCacheStorage#listDigests. #898

Merged

loosebazooka reviewed Aug 31, 2018

View reviewed changes

Consolidates file locking and feedback.

0e92e58

coollog requested a review from a team September 5, 2018 18:19

TadCordle reviewed Sep 5, 2018

View reviewed changes

coollog added 3 commits September 6, 2018 10:26

feedback

b0d02d9

Reorganizes the write to be atomic with regards to the layer directory.

b0ebeac

Adds TemporaryDirectory.

142cbb8

coollog added 3 commits September 6, 2018 15:34

Merges master.

f8994e4

Merge branch 'master' into ncache-3-defaultcachestorage

170e962

Don't write temporary to cache directory.

32567e4

loosebazooka reviewed Sep 7, 2018

View reviewed changes

feedback

a894882

coollog requested a review from a team September 10, 2018 17:50

loosebazooka approved these changes Sep 10, 2018

View reviewed changes

coollog merged commit 411da83 into master Sep 10, 2018

coollog deleted the ncache-3-defaultcachestorage branch September 10, 2018 20:17

loosebazooka reviewed Sep 10, 2018

View reviewed changes

briandealwis mentioned this pull request Mar 25, 2019

GitLab runner cannot delete jib-cache folder starting with v.0.10.0 #1575

Closed

emmileaf mentioned this pull request Sep 9, 2022

Check for existence of layers in cache before returning cached base image #3767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds DefaultCacheStorage#write. #879

Adds DefaultCacheStorage#write. #879

coollog commented Aug 24, 2018 •

edited

Loading

loosebazooka Aug 31, 2018

loosebazooka Aug 31, 2018

coollog Sep 4, 2018

loosebazooka Aug 31, 2018

TadCordle Sep 5, 2018

TadCordle Sep 5, 2018

TadCordle Sep 5, 2018

coollog Sep 6, 2018

TadCordle Sep 5, 2018

coollog Sep 6, 2018

coollog commented Sep 6, 2018

loosebazooka Sep 7, 2018

loosebazooka left a comment

coollog commented Sep 10, 2018

loosebazooka Sep 10, 2018

coollog Sep 10, 2018


		private final DefaultCacheStorageWriter defaultCacheStorageWriter;

		private DefaultCacheStorage(Path cacheDirectory) {

Adds DefaultCacheStorage#write. #879

Adds DefaultCacheStorage#write. #879

Conversation

coollog commented Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog commented Sep 6, 2018

Choose a reason for hiding this comment

loosebazooka left a comment

Choose a reason for hiding this comment

coollog commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog commented Aug 24, 2018 •

edited

Loading