Skip to content

Commit 5fc91a7

Browse files
buchgrcopybara-github
buchgr
authored and
copybara-github
committed
Remove contains() and containsActionResult from SimpleBlobStore
contains() and containsActionResult() only make sense for the OnDiskBlobStore because they can be implemented with a (quickly returning) stat() call. They are not generally useful for remote caching/execution. Thus this change moves them down to the OnDiskBlobStore. In the same breath I removed any non-test uses of the InMemoryBlobStore and moved the class to the test package. The InMemoryBlobStore has never been used by Bazel itself but only by its tests and the LRE. The LRE usage has never really made sense either and so I removed it from there as well. Closes #9173. PiperOrigin-RevId: 264590465
1 parent 209175f commit 5fc91a7

17 files changed

Lines changed: 147 additions & 253 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@
4848
* <p>Note that this class is used from src/tools/remote.
4949
*/
5050
@ThreadSafe
51-
public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache {
52-
private final SimpleBlobStore blobStore;
51+
public class SimpleBlobStoreActionCache extends AbstractRemoteActionCache {
52+
protected final SimpleBlobStore blobStore;
5353

5454
public SimpleBlobStoreActionCache(
5555
RemoteOptions options, SimpleBlobStore blobStore, DigestUtil digestUtil) {
@@ -86,10 +86,6 @@ protected ImmutableSet<Digest> getMissingDigests(Iterable<Digest> digests) {
8686
return ImmutableSet.copyOf(digests);
8787
}
8888

89-
public boolean containsKey(Digest digest) throws IOException, InterruptedException {
90-
return blobStore.contains(digest.getHash());
91-
}
92-
9389
@Override
9490
public ActionResult getCachedActionResult(ActionKey actionKey)
9591
throws IOException, InterruptedException {

‎src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import com.google.common.base.Ascii;
1919
import com.google.common.base.Preconditions;
2020
import com.google.common.collect.ImmutableList;
21-
import com.google.devtools.build.lib.remote.blobstore.ConcurrentMapBlobStore;
2221
import com.google.devtools.build.lib.remote.common.SimpleBlobStore;
2322
import com.google.devtools.build.lib.remote.disk.CombinedDiskHttpBlobStore;
2423
import com.google.devtools.build.lib.remote.disk.OnDiskBlobStore;
@@ -29,7 +28,6 @@
2928
import io.netty.channel.unix.DomainSocketAddress;
3029
import java.io.IOException;
3130
import java.net.URI;
32-
import java.util.concurrent.ConcurrentHashMap;
3331
import javax.annotation.Nullable;
3432

3533
/**
@@ -46,7 +44,7 @@ public static SimpleBlobStore create(RemoteOptions remoteOptions, @Nullable Path
4644
} else if (casPath != null) {
4745
return new OnDiskBlobStore(casPath);
4846
} else {
49-
return new ConcurrentMapBlobStore(new ConcurrentHashMap<>());
47+
return null;
5048
}
5149
}
5250

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

Lines changed: 0 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -13,93 +13,5 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.remote.blobstore;
1515

16-
import build.bazel.remote.execution.v2.ActionResult;
17-
import build.bazel.remote.execution.v2.Digest;
18-
import com.google.common.base.Preconditions;
19-
import com.google.common.io.ByteStreams;
20-
import com.google.common.util.concurrent.Futures;
21-
import com.google.common.util.concurrent.ListenableFuture;
22-
import com.google.common.util.concurrent.SettableFuture;
23-
import com.google.devtools.build.lib.remote.common.SimpleBlobStore;
24-
import com.google.devtools.build.lib.vfs.Path;
25-
import com.google.protobuf.ByteString;
26-
import java.io.IOException;
27-
import java.io.InputStream;
28-
import java.io.OutputStream;
29-
import java.util.concurrent.ConcurrentMap;
3016

3117
/** A {@link SimpleBlobStore} implementation using a {@link ConcurrentMap}. */
32-
public final class ConcurrentMapBlobStore implements SimpleBlobStore {
33-
private final ConcurrentMap<String, byte[]> map;
34-
static final String ACTION_KEY_PREFIX = "ac_";
35-
36-
public ConcurrentMapBlobStore(ConcurrentMap<String, byte[]> map) {
37-
this.map = map;
38-
}
39-
40-
@Override
41-
public boolean contains(String key) {
42-
return map.containsKey(key);
43-
}
44-
45-
@Override
46-
public boolean containsActionResult(String key) {
47-
return map.containsKey(ACTION_KEY_PREFIX + key);
48-
}
49-
50-
@Override
51-
public ListenableFuture<Boolean> get(String key, OutputStream out) {
52-
byte[] data = map.get(key);
53-
SettableFuture<Boolean> f = SettableFuture.create();
54-
if (data == null) {
55-
f.set(false);
56-
} else {
57-
try {
58-
out.write(data);
59-
f.set(true);
60-
} catch (IOException e) {
61-
f.setException(e);
62-
}
63-
}
64-
return f;
65-
}
66-
67-
@Override
68-
public ListenableFuture<Boolean> getActionResult(String key, OutputStream out) {
69-
return get(ACTION_KEY_PREFIX + key, out);
70-
}
71-
72-
@Override
73-
public void putActionResult(ActionKey actionKey, ActionResult actionResult) {
74-
map.put(ACTION_KEY_PREFIX + actionKey.getDigest().getHash(), actionResult.toByteArray());
75-
}
76-
77-
@Override
78-
public void close() {}
79-
80-
@Override
81-
public ListenableFuture<Void> uploadFile(Digest digest, Path file) {
82-
try (InputStream in = file.getInputStream()) {
83-
upload(digest.getHash(), digest.getSizeBytes(), in);
84-
} catch (IOException e) {
85-
return Futures.immediateFailedFuture(e);
86-
}
87-
return Futures.immediateFuture(null);
88-
}
89-
90-
@Override
91-
public ListenableFuture<Void> uploadBlob(Digest digest, ByteString data) {
92-
try (InputStream in = data.newInput()) {
93-
upload(digest.getHash(), digest.getSizeBytes(), in);
94-
} catch (IOException e) {
95-
return Futures.immediateFailedFuture(e);
96-
}
97-
return Futures.immediateFuture(null);
98-
}
99-
100-
private void upload(String key, long length, InputStream in) throws IOException {
101-
byte[] value = ByteStreams.toByteArray(in);
102-
Preconditions.checkState(value.length == length);
103-
map.put(key, value);
104-
}
105-
}

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

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,6 @@ public ActionKey(Digest digest) {
4545
}
4646
}
4747

48-
/** Returns {@code true} if the provided {@code key} is stored in the CAS. */
49-
boolean contains(String key) throws IOException, InterruptedException;
50-
51-
/** Returns {@code true} if the provided {@code key} is stored in the Action Cache. */
52-
boolean containsActionResult(String key) throws IOException, InterruptedException;
53-
5448
/**
5549
* Fetches the BLOB associated with the {@code key} from the CAS and writes it to {@code out}.
5650
*

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,6 @@ public CombinedDiskHttpBlobStore(OnDiskBlobStore diskCache, SimpleBlobStore remo
4545
this.remoteCache = Preconditions.checkNotNull(remoteCache);
4646
}
4747

48-
@Override
49-
public boolean contains(String key) {
50-
return diskCache.contains(key);
51-
}
52-
53-
@Override
54-
public boolean containsActionResult(String key) {
55-
return diskCache.containsActionResult(key);
56-
}
57-
5848
@Override
5949
public void putActionResult(ActionKey actionKey, ActionResult actionResult)
6050
throws IOException, InterruptedException {

‎src/main/java/com/google/devtools/build/lib/remote/disk/OnDiskBlobStore.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@ public OnDiskBlobStore(Path root) {
3636
this.root = root;
3737
}
3838

39-
@Override
39+
/** Returns {@code true} if the provided {@code key} is stored in the CAS. */
4040
public boolean contains(String key) {
4141
return toPath(key, /* actionResult= */ false).exists();
4242
}
4343

44-
@Override
44+
/** Returns {@code true} if the provided {@code key} is stored in the Action Cache. */
4545
public boolean containsActionResult(String key) {
4646
return toPath(key, /* actionResult= */ true).exists();
4747
}

‎src/main/java/com/google/devtools/build/lib/remote/http/HttpBlobStore.java‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -415,16 +415,6 @@ private boolean isChannelPipelineEmpty(ChannelPipeline pipeline) {
415415
&& pipeline.first() == pipeline.last());
416416
}
417417

418-
@Override
419-
public boolean contains(String key) {
420-
throw new UnsupportedOperationException("HTTP Caching does not use this method.");
421-
}
422-
423-
@Override
424-
public boolean containsActionResult(String key) {
425-
throw new UnsupportedOperationException("HTTP Caching does not use this method.");
426-
}
427-
428418
@Override
429419
public ListenableFuture<Boolean> get(String key, OutputStream out) {
430420
return get(key, out, true);

‎src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java‎

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,17 @@
2727
import build.bazel.remote.execution.v2.FileNode;
2828
import build.bazel.remote.execution.v2.Tree;
2929
import com.google.common.base.Charsets;
30+
import com.google.common.base.Preconditions;
3031
import com.google.common.collect.ImmutableList;
32+
import com.google.common.io.ByteStreams;
33+
import com.google.common.util.concurrent.Futures;
34+
import com.google.common.util.concurrent.ListenableFuture;
3135
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
3236
import com.google.common.util.concurrent.MoreExecutors;
37+
import com.google.common.util.concurrent.SettableFuture;
3338
import com.google.devtools.build.lib.actions.ActionInputHelper;
3439
import com.google.devtools.build.lib.clock.JavaClock;
35-
import com.google.devtools.build.lib.remote.blobstore.ConcurrentMapBlobStore;
40+
import com.google.devtools.build.lib.remote.common.SimpleBlobStore;
3641
import com.google.devtools.build.lib.remote.common.SimpleBlobStore.ActionKey;
3742
import com.google.devtools.build.lib.remote.options.RemoteOptions;
3843
import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -44,8 +49,11 @@
4449
import com.google.devtools.build.lib.vfs.Path;
4550
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
4651
import com.google.devtools.common.options.Options;
52+
import com.google.protobuf.ByteString;
4753
import io.grpc.Context;
4854
import java.io.IOException;
55+
import java.io.InputStream;
56+
import java.io.OutputStream;
4957
import java.util.List;
5058
import java.util.concurrent.ConcurrentHashMap;
5159
import java.util.concurrent.ConcurrentMap;
@@ -434,4 +442,69 @@ public void testDownloadFailsOnDigestMismatch() {
434442
() -> getFromFuture(client.downloadFile(fs.getPath("/exec/root/foo"), digest)));
435443
assertThat(e).hasMessageThat().contains(digest.getHash());
436444
}
445+
446+
private static class ConcurrentMapBlobStore implements SimpleBlobStore {
447+
private final ConcurrentMap<String, byte[]> map;
448+
private static final String ACTION_KEY_PREFIX = "ac_";
449+
450+
public ConcurrentMapBlobStore(ConcurrentMap<String, byte[]> map) {
451+
this.map = map;
452+
}
453+
454+
@Override
455+
public ListenableFuture<Boolean> get(String key, OutputStream out) {
456+
byte[] data = map.get(key);
457+
SettableFuture<Boolean> f = SettableFuture.create();
458+
if (data == null) {
459+
f.set(false);
460+
} else {
461+
try {
462+
out.write(data);
463+
f.set(true);
464+
} catch (IOException e) {
465+
f.setException(e);
466+
}
467+
}
468+
return f;
469+
}
470+
471+
@Override
472+
public ListenableFuture<Boolean> getActionResult(String key, OutputStream out) {
473+
return get(ACTION_KEY_PREFIX + key, out);
474+
}
475+
476+
@Override
477+
public void putActionResult(ActionKey actionKey, ActionResult actionResult) {
478+
map.put(ACTION_KEY_PREFIX + actionKey.getDigest().getHash(), actionResult.toByteArray());
479+
}
480+
481+
@Override
482+
public void close() {}
483+
484+
@Override
485+
public ListenableFuture<Void> uploadFile(Digest digest, Path file) {
486+
try (InputStream in = file.getInputStream()) {
487+
upload(digest.getHash(), digest.getSizeBytes(), in);
488+
} catch (IOException e) {
489+
return Futures.immediateFailedFuture(e);
490+
}
491+
return Futures.immediateFuture(null);
492+
}
493+
494+
@Override
495+
public ListenableFuture<Void> uploadBlob(Digest digest, ByteString data) {
496+
try (InputStream in = data.newInput()) {
497+
upload(digest.getHash(), digest.getSizeBytes(), in);
498+
} catch (IOException e) {
499+
return Futures.immediateFailedFuture(e);
500+
}
501+
return Futures.immediateFuture(null);
502+
}
503+
504+
private void upload(String key, long length, InputStream in) throws IOException {
505+
byte[] value = ByteStreams.toByteArray(in);
506+
Preconditions.checkState(value.length == length);
507+
map.put(key, value);
508+
}
509+
}
437510
}

‎src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactoryTest.java‎

Lines changed: 0 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
1919

2020
import com.google.devtools.build.lib.clock.JavaClock;
21-
import com.google.devtools.build.lib.remote.blobstore.ConcurrentMapBlobStore;
2221
import com.google.devtools.build.lib.remote.common.SimpleBlobStore;
2322
import com.google.devtools.build.lib.remote.disk.CombinedDiskHttpBlobStore;
2423
import com.google.devtools.build.lib.remote.disk.OnDiskBlobStore;
@@ -210,70 +209,4 @@ public void isRemoteCacheOptions_remoteHttpCacheOptionEmpty() {
210209
public void isRemoteCacheOptions_defaultOptions() {
211210
assertThat(SimpleBlobStoreFactory.isRemoteCacheOptions(remoteOptions)).isFalse();
212211
}
213-
214-
@Test
215-
public void create_httpCacheWhenHttpAndDiskCacheEnabled() {
216-
remoteOptions.remoteCache = "http://doesnotexist.com";
217-
remoteOptions.diskCache = PathFragment.create("/etc/something/cache/here");
218-
219-
SimpleBlobStore blobStore = SimpleBlobStoreFactory.create(remoteOptions, /* casPath= */ null);
220-
221-
assertThat(blobStore).isInstanceOf(HttpBlobStore.class);
222-
}
223-
224-
@Test
225-
public void create_httpCacheWithProxy() {
226-
remoteOptions.remoteCache = "http://doesnotexist.com";
227-
remoteOptions.remoteProxy = "unix://some-proxy";
228-
229-
SimpleBlobStore blobStore = SimpleBlobStoreFactory.create(remoteOptions, /* casPath= */ null);
230-
231-
assertThat(blobStore).isInstanceOf(HttpBlobStore.class);
232-
}
233-
234-
@Test
235-
public void create_httpCacheFailsWithUnsupportedProxyProtocol() {
236-
remoteOptions.remoteCache = "http://doesnotexist.com";
237-
remoteOptions.remoteProxy = "bad-proxy";
238-
239-
assertThat(
240-
assertThrows(
241-
Exception.class,
242-
() -> SimpleBlobStoreFactory.create(remoteOptions, /* casPath= */ null)))
243-
.hasMessageThat()
244-
.contains("Remote cache proxy unsupported: bad-proxy");
245-
}
246-
247-
@Test
248-
public void create_httpCacheWithoutProxy() {
249-
remoteOptions.remoteCache = "http://doesnotexist.com";
250-
251-
SimpleBlobStore blobStore = SimpleBlobStoreFactory.create(remoteOptions, /* casPath= */ null);
252-
253-
assertThat(blobStore).isInstanceOf(HttpBlobStore.class);
254-
}
255-
256-
@Test
257-
public void create_diskCacheWithCasPath() {
258-
SimpleBlobStore blobStore =
259-
SimpleBlobStoreFactory.create(remoteOptions, fs.getPath("/cas/path/is/here"));
260-
261-
assertThat(blobStore).isInstanceOf(OnDiskBlobStore.class);
262-
}
263-
264-
@Test
265-
public void create_defaultCacheWhenDiskCacheEnabled() {
266-
remoteOptions.diskCache = PathFragment.create("/etc/something/cache/here");
267-
268-
SimpleBlobStore blobStore = SimpleBlobStoreFactory.create(remoteOptions, /* casPath= */ null);
269-
270-
assertThat(blobStore).isInstanceOf(ConcurrentMapBlobStore.class);
271-
}
272-
273-
@Test
274-
public void create_defaultCache() {
275-
SimpleBlobStore blobStore = SimpleBlobStoreFactory.create(remoteOptions, null);
276-
277-
assertThat(blobStore).isInstanceOf(ConcurrentMapBlobStore.class);
278-
}
279212
}

0 commit comments

Comments
 (0)