remote: made CombinedCache a composition of Disk and Http Cache #8141

ishikhman · 2019-04-25T13:59:16Z

The problem: CombinedCache is not working.
The cause: both OnDiskBlobStore and HttpBlobStore store action results differently from other actions, but not in the same way. The OnDiskBlobStore appends "ac_" prefix to the key, while the HttpBlobStore stores it in a different directory. Current implementation added "ac_" prefix for both blob stores, which is not correct for the HttpBlobStore.
The solution: treat actionResults' gets and puts in accordance with a particular Blob Store used.

The problem is described in #8052 and the root cause is described in this and this comments.

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

buchgr · 2019-04-29T11:07:35Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

 /**
 * A {@link SimpleBlobStore} implementation combining two blob stores. A local disk blob store and a
 * remote http blob store. If a blob isn't found in the first store, the second store is used, and
 * the blob added to the first. Put puts the blob on both stores.
 */
-public final class CombinedDiskHttpBlobStore extends OnDiskBlobStore {
+public final class CombinedDiskHttpBlobStore implements SimpleBlobStore {


I don't understand what this buys us. The code is still closely tight to the local disk cache by having copied the move logic in get. I couldn't possibly plug in anything except the disk cache there?

Right, it is quite tight to the local disk cache, but it has a modified not just copied logic. I'd say there is not much difference in this particular case, so I'd stay with composition (it is done already and did not take more than 2 minutes of implementation time ;)) .

buchgr · 2019-04-29T11:08:09Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

+  private final OnDiskBlobStore diskCache;
+  private final Path root;
+
+  public CombinedDiskHttpBlobStore(SimpleBlobStore diskCache, SimpleBlobStore httpCache, Path root) {


why not OnDiskBlobStore?

Fair point. Changed.

ishikhman

Pushed the updated version. Will think about merging Combined Cache with Disk Cache.

ishikhman · 2019-04-29T14:22:02Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

+  private final OnDiskBlobStore diskCache;
+  private final Path root;
+
+  public CombinedDiskHttpBlobStore(SimpleBlobStore diskCache, SimpleBlobStore httpCache, Path root) {


Fair point. Changed.

ishikhman · 2019-04-29T14:23:25Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

 /**
 * A {@link SimpleBlobStore} implementation combining two blob stores. A local disk blob store and a
 * remote http blob store. If a blob isn't found in the first store, the second store is used, and
 * the blob added to the first. Put puts the blob on both stores.
 */
-public final class CombinedDiskHttpBlobStore extends OnDiskBlobStore {
+public final class CombinedDiskHttpBlobStore implements SimpleBlobStore {


Right, it is quite tight to the local disk cache, but it has a modified not just copied logic. I'd say there is not much difference in this particular case, so I'd stay with composition (it is done already and did not take more than 2 minutes of implementation time ;)) .

ishikhman · 2019-04-30T10:43:34Z

@buchgr, PTAL.
I tried to merge DiskStore and CombinedDiskStore - it looked much more complex than it is now, because every method had to be there 3 times: 1 public, 1 private for actual interactions with disk store, 1 private for interactions with Remote Store. The class became big and not readable. So, i'd stay with the current solution.

buchgr · 2019-04-30T15:33:43Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

+  }
+
+  private ListenableFuture<Boolean> get(String key, OutputStream out, boolean actionResult) {
+    boolean foundOnDisk = diskCache.toPath(key, actionResult).exists();


why not call contains here?

because it's not aware of the 'actionResult' flag, and I don't want to change an interface at this very moment

I think it would be cleaner to update the interface.

buchgr · 2019-05-02T09:10:43Z

Can you please add a test for the regression?

buchgr · 2019-05-02T09:35:02Z

Also, please update the commit message as discussed.

well, actually fixed RemoteWorker to make regression tests failing for erroneous solution and pass for the fix

buchgr

mostly lgtm. just some smaller things. thanks!

buchgr · 2019-05-06T09:33:07Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java

+
+  public CombinedDiskHttpBlobStore(OnDiskBlobStore diskCache, SimpleBlobStore remoteCache) {
+    this.diskCache = Preconditions.checkNotNull(diskCache);
+    this.remoteCache = Preconditions.checkNotNull(remoteCache);
  }

  @Override
  public boolean containsKey(String key) {


call this only contains to be in line with get now that you introduced containsActionResult?

buchgr · 2019-05-06T09:35:02Z

src/main/java/com/google/devtools/build/lib/remote/blobstore/SimpleBlobStore.java

@@ -30,6 +30,11 @@
   */
  boolean containsKey(String key) throws IOException, InterruptedException;

+  /**
+   * Returns {@code key} if the provided {@code key} is stored in the Action Cache.


s/key/true/

buchgr · 2019-05-06T09:37:02Z

...remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

@@ -0,0 +1,38 @@
+package com.google.devtools.build.remote.worker;


add license

buchgr · 2019-05-06T09:37:04Z

src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD

+)
+
+java_test(
+    name = "test-remote-worker",


s/test-remote-worker/worker/

buchgr · 2019-05-06T09:39:10Z

...remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

+  private HttpCacheServerHandler handler = new HttpCacheServerHandler();
+
+  @Test
+  public void isUriValidWhenValidUri() {


testValidUri

buchgr · 2019-05-06T09:39:25Z

...remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

+  }
+
+  @Test
+  public void isUriValidWhenInvalidUri() {


testInvalidUri

buchgr · 2019-05-06T09:43:33Z

...remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

+
+  @Test
+  public void isUriValidWhenValidUri() {
+    assertTrue(handler.isUriValid("http://some-path.co.uk:8080/ac/1111111111111111111111111111111111111111111111111111111111111111"));


Use an actual hash here. Maybe the one for the empty file e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

buchgr · 2019-05-06T09:48:19Z

...ols/remote/src/main/java/com/google/devtools/build/remote/worker/HttpCacheServerHandler.java

@@ -54,7 +57,19 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request)
    }
  }

+  @VisibleForTesting
+  boolean isUriValid(String uri) {


buchgr · 2019-05-06T09:49:03Z

...ols/remote/src/main/java/com/google/devtools/build/remote/worker/HttpCacheServerHandler.java

+  @VisibleForTesting
+  boolean isUriValid(String uri) {
+    String URL_PATTERN = "^/?(.*/)?(ac/|cas/)([a-f0-9]{64})$";
+    Matcher matcher = Pattern.compile(URL_PATTERN).matcher(uri);


put Pattern.compile(URL_PATTERN) in a field and compute only once?

buchgr · 2019-05-06T09:49:32Z

...remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

+    assertTrue(handler.isUriValid("http://localhost:8080/cas/1111111111111111111111111111111111111111111111111111111111111111"));
+    assertTrue(handler.isUriValid("https://localhost:8080/cas/1111111111111111111111111111111111111111111111111111111111111111"));
+    assertTrue(handler.isUriValid("localhost:8080/cas/1111111111111111111111111111111111111111111111111111111111111111"));
+  }


IIRC HttpRequest.getUri() for servers will return the path. So maybe test cas/hash too.

buchgr · 2019-05-07T08:24:10Z

LGTM

buchgr · 2019-05-07T08:25:06Z

The srcs test is failing. You need to add the srcs filegroup to the //:srcs target.

//:srcs filegroup do not contains all the sources, missing:
src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD
src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

ishikhman · 2019-05-08T08:14:53Z

The srcs test is failing. You need to add the srcs filegroup to the //:srcs target.

//:srcs filegroup do not contains all the sources, missing:
src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD
src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java

Thanks!

Fixes #8052 The problem: CombinedCache is not working. The cause: both `OnDiskBlobStore` and `HttpBlobStore` store action results differently from other actions, but not in the same way. The `OnDiskBlobStore` appends "ac_" prefix to the key, while the `HttpBlobStore` stores it in a different directory. Current implementation added "ac_" prefix for both blob stores, which is not correct for the `HttpBlobStore`. The solution: treat actionResults' `gets` and `puts` in accordance with a particular Blob Store used. The problem is described in #8052 and the root cause is described in [this](#8052 (comment)) and [this](#8052 (comment)) comments. Closes #8141. PiperOrigin-RevId: 247387377

Fixes bazelbuild#8052 The problem: CombinedCache is not working. The cause: both `OnDiskBlobStore` and `HttpBlobStore` store action results differently from other actions, but not in the same way. The `OnDiskBlobStore` appends "ac_" prefix to the key, while the `HttpBlobStore` stores it in a different directory. Current implementation added "ac_" prefix for both blob stores, which is not correct for the `HttpBlobStore`. The solution: treat actionResults' `gets` and `puts` in accordance with a particular Blob Store used. The problem is described in bazelbuild#8052 and the root cause is described in [this](bazelbuild#8052 (comment)) and [this](bazelbuild#8052 (comment)) comments. Closes bazelbuild#8141. PiperOrigin-RevId: 247387377

googlebot added the cla: yes label Apr 25, 2019

ishikhman commented Apr 25, 2019

View reviewed changes

src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java Show resolved Hide resolved

ishikhman mentioned this pull request Apr 25, 2019

Remote cache + disk cache requests incorrect action keys #8052

Closed

aherrmann reviewed Apr 25, 2019

View reviewed changes

ishikhman force-pushed the ishikhman-8052 branch from 83eb90b to 5927a12 Compare April 26, 2019 15:27

fixed combined cache to behave correctly for action results

b84f14d

ishikhman force-pushed the ishikhman-8052 branch from 5927a12 to b84f14d Compare April 29, 2019 08:31

ishikhman closed this Apr 29, 2019

ishikhman reopened this Apr 29, 2019

cleaned the code

69d478f

ishikhman changed the title ~~draft - made CombinedCache a composition of Disk and Http~~ remote: made CombinedCache a composition of Disk and Http Cache Apr 29, 2019

ishikhman marked this pull request as ready for review April 29, 2019 10:48

ishikhman requested review from buchgr and ola-rozenfeld as code owners April 29, 2019 10:48

buchgr reviewed Apr 29, 2019

View reviewed changes

made the solution more generic

1c41421

ishikhman commented Apr 29, 2019

View reviewed changes

kastiglione mentioned this pull request Apr 29, 2019

Change execution log proto to differentiate between remote and disk cache hits #8192

Closed

split big method into smaller steps

73d5b24

ishikhman added 2 commits April 30, 2019 16:27

renamed back

e10bce3

located diskCache key modification logic inside onDiskBlobStore only

08a5c42

ishikhman force-pushed the ishikhman-8052 branch from 78cf7e7 to 08a5c42 Compare April 30, 2019 14:59

buchgr reviewed Apr 30, 2019

View reviewed changes

buchgr mentioned this pull request May 2, 2019

Malformed /ac/ url when both http and disk caching enabled #8218

Closed

added 'containsActionResult' method

4eea91b

ishikhman force-pushed the ishikhman-8052 branch from 8d7a3e5 to 797fbfb Compare May 6, 2019 08:20

added regression tests

543805f

well, actually fixed RemoteWorker to make regression tests failing for erroneous solution and pass for the fix

ishikhman force-pushed the ishikhman-8052 branch from 797fbfb to 543805f Compare May 6, 2019 09:39

buchgr reviewed May 6, 2019

View reviewed changes

addressed review comments

841bd47

irengrig added the WIP label May 7, 2019

fixed srcs test

c9cf4f5

bazel-io closed this in 597e289 May 9, 2019

		@@ -0,0 +1,38 @@
		package com.google.devtools.build.remote.worker;

remote: made CombinedCache a composition of Disk and Http Cache #8141

remote: made CombinedCache a composition of Disk and Http Cache #8141

Conversation

ishikhman commented Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchgr Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishikhman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishikhman commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchgr commented May 2, 2019

buchgr commented May 2, 2019

buchgr left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchgr commented May 7, 2019

buchgr commented May 7, 2019

ishikhman commented May 8, 2019

ishikhman commented Apr 25, 2019 •

edited

Loading

buchgr Apr 29, 2019 •

edited

Loading