Refactor SimpleBlobStore#get(ActionResult)(...)#9200
Refactor SimpleBlobStore#get(ActionResult)(...)#9200buchgr wants to merge 1 commit intobazelbuild:masterfrom
Conversation
| CacheNotFoundException(Digest missingDigest, DigestUtil digestUtil) { | ||
| super("Missing digest: " + digestUtil.toString(missingDigest)); | ||
| public CacheNotFoundException(Digest missingDigest) { | ||
| super("Missing digest: " + missingDigest.getHash() + "/" + missingDigest.getSizeBytes()); |
There was a problem hiding this comment.
| super("Missing digest: " + missingDigest.getHash() + "/" + missingDigest.getSizeBytes()); | |
| super("Missing digest: " + DigestUtil.toString(missingDigest)); |
There was a problem hiding this comment.
Same reason as above :. I added a TODO.
| resourceName += options.remoteInstanceName + "/"; | ||
| } | ||
| resourceName += "blobs/" + digestUtil.toString(digest); | ||
| resourceName += "blobs/" + digest.getHash() + "/" + digest.getSizeBytes(); |
There was a problem hiding this comment.
| resourceName += "blobs/" + digest.getHash() + "/" + digest.getSizeBytes(); | |
| resourceName += "blobs/" + DigestUtil.toString(digest); |
There was a problem hiding this comment.
This would introduce dependency cycles. However, it seems like a small price to pay.
| assertThat(e).hasMessageThat().contains(digest.getHash()); | ||
| } | ||
|
|
||
| private class ConcurrentMapBlobStore implements SimpleBlobStore { |
There was a problem hiding this comment.
supernit: Maybe extract it into a separate file in test sources? Just to keep this file smaller :)
Could be package private if reusability is a concern
| } | ||
|
|
||
| public boolean containsKey(Digest digest) { | ||
| return ((OnDiskBlobStore) blobStore).contains(digest); |
There was a problem hiding this comment.
Maybe add a private final field that's already casted instead of casting every time?
I'm afraid casting every time will have runtime overhead before (if) JIT figures out that cast always succeeds, given how often containsKey will be called in large builds
Docs: checkcast
There was a problem hiding this comment.
Good point in general but this code is not performance critical as it's only used in tests.
There was a problem hiding this comment.
Still, it could be a good idea to 'not' make tests run longer ;)
buchgr
force-pushed
the
refactor-download
branch
2 times, most recently
from
494f06a to
ba89d13
Compare
|
@buchgr Is it also related to making GrpcRemoteCache a SimpleBlobStore implementation? |
|
@borkaehw yes |
The change update methods signatures to be compatible with those in AbstractRemoteActionCache: 1) Future<Boolean> get(String, OutputStream) -> Future<Void> downloadBlob(Digest, OutputStream) 2) Future<Boolean> getActionResult(String, OutputStream) -> Future<ActionResult> downloadActionResult(ActionKey) The refactoring uncovered a bug in CombinedDiskHttpBlobStore. We did not close the output stream *before* moving a file to the disk cache, thus potentially moving a corrupted file to the disk cache as any errors would only be reported afterwards.
buchgr
force-pushed
the
refactor-download
branch
from
ba89d13 to
fefd706
Compare
buchgr
changed the title
| } | ||
|
|
||
| public boolean containsKey(Digest digest) { | ||
| return ((OnDiskBlobStore) blobStore).contains(digest); |
There was a problem hiding this comment.
Still, it could be a good idea to 'not' make tests run longer ;)
They won't. This is a micro optimization that may shave off a few micro seconds. |
|
@buchgr what's the status on this PR? |
|
@SrodriguezO it's on a way to be merged, but @buchgr is away for a few weeks, so it will be finalised in a week or so |
|
@buchgr friendly ping : ) |
The change update methods signatures to be compatible with those in AbstractRemoteActionCache: 1) Future<Boolean> get(String, OutputStream) -> Future<Void> downloadBlob(Digest, OutputStream) 2) Future<Boolean> getActionResult(String, OutputStream) -> Future<ActionResult> downloadActionResult(ActionKey) The refactoring uncovered a bug in CombinedDiskHttpBlobStore. We did not close the output stream *before* moving a file to the disk cache, thus potentially moving a corrupted file to the disk cache as any errors would only be reported afterwards. Closes bazelbuild#9200. PiperOrigin-RevId: 278830531
7 participants
The change update methods signatures to be compatible with those in
AbstractRemoteActionCache:
The refactoring uncovered a bug in CombinedDiskHttpBlobStore. We did not
close the output stream before moving a file to the disk cache, thus
potentially moving a corrupted file to the disk cache as any errors
would only be reported afterwards.