Skip to content

Remove contains() and containsActionResult from SimpleBlobStore#9173

Closed
buchgr wants to merge 1 commit intobazelbuild:masterfrom
buchgr:remove-contains
Closed

Remove contains() and containsActionResult from SimpleBlobStore#9173
buchgr wants to merge 1 commit intobazelbuild:masterfrom
buchgr:remove-contains

Conversation

@buchgr
Copy link
Copy Markdown
Contributor

@buchgr buchgr commented Aug 14, 2019

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.

@buchgr
Copy link
Copy Markdown
Contributor Author

buchgr commented Aug 14, 2019

FYI @borkaehw

After this change is in the only change remaining to be able to get a GrpcBlobStore and remove GrpcRemoteCache and SimpleBlobStoreActionCache is to refactor SimpleBlobStore to

ListenableFuture<Boolean> get(String key, OutputStream out) -> ListenableFuture<Void> get(Digest digest, OutputStream out)

and

ListenableFuture<Boolean> getActionResult(String actionKey, OutputStream out) -> ListenableFuture<ActionResult> getActionResult(ActionKey actionKey).

Copy link
Copy Markdown
Contributor

@ishikhman ishikhman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@buchgr buchgr force-pushed the remove-contains branch 2 times, most recently from ddad4b7 to d84d58a Compare August 20, 2019 07:55
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants