Skip to content

Refactor SimpleBlobStore#put#9028

Closed
buchgr wants to merge 3 commits intobazelbuild:masterfrom
buchgr:remove-put
Closed

Refactor SimpleBlobStore#put#9028
buchgr wants to merge 3 commits intobazelbuild:masterfrom
buchgr:remove-put

Conversation

@buchgr
Copy link
Copy Markdown
Contributor

@buchgr buchgr commented Jul 31, 2019

Split it into SimpleBlobStore#uploadFile and SimpleBlobStore#uploadBlob.
The return type changes to ListenableFuture<Void> but all
implementations are still blocking for now.

This is a refactoring towards the larger goal of removing SimpleBlobStoreActionCache
and making GrpcRemoteCache a SimpleBlobStore implementation.

@buchgr
Copy link
Copy Markdown
Contributor Author

buchgr commented Jul 31, 2019

@borkaehw

@borkaehw
Copy link
Copy Markdown

Do we have an ETA for this PR?

@SrodriguezO
Copy link
Copy Markdown

SimpleBlobStoreActionCache extends AbstractRemoteActionCache, which is what the cache is abstracted away to in RemoteModule.java.

If we're removing SimpleBlobStoreActionCache, is the plan to have SimpleBlobStore extend AbstractRemoteActionCache directly?

@buchgr
Copy link
Copy Markdown
Contributor Author

buchgr commented Aug 1, 2019

@SrodriguezO the plan is for the AbstractRemoteActionCache to no longer be abstract but instead for it to has-a SimpleBlobStore. Grpc, Http and Disk should all be of type SimpleBlobStore.

The do the following renames:

AbstractRemoteActionCache -> RemoteCache
SimpleBlobStore -> RemoteCacheProtocol

@SrodriguezO
Copy link
Copy Markdown

Ah that makes sense. Sounds great! Thanks for clarifying

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.

thanks for this refactoring!
a few notes from me below.

Comment thread src/main/java/com/google/devtools/build/lib/remote/http/HttpBlobStore.java Outdated
@irengrig irengrig added the WIP label Aug 2, 2019
buchgr added 2 commits August 5, 2019 11:43
Split it into SimpleBlobStore#uploadFile and SimpleBlobStore#uploadBlob.
The return type changes to ListenableFuture<Void> but all
implementations are still blocking for now. This will be changed in a
follow up CL.

This is a refactoring towards the larger goal of removing SimpleBlobStoreActionCache
and making GrpcRemoteCache a SimpleBlobStore implementation. The idea
is for AbstractRemoteActionCache#upload(ActionResult, ...) to no longer
be abstract but instead to call out to abstract uploadFile and
uploadBlob methods (just like downloadBlob).
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

@bazel-io bazel-io closed this in 36611c3 Aug 12, 2019
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.

6 participants