Skip to content

move SimpleBlobStore to common package#8972

Closed
buchgr wants to merge 3 commits intobazelbuild:masterfrom
buchgr:refactor-blobstore
Closed

move SimpleBlobStore to common package#8972
buchgr wants to merge 3 commits intobazelbuild:masterfrom
buchgr:refactor-blobstore

Conversation

@buchgr
Copy link
Copy Markdown
Contributor

@buchgr buchgr commented Jul 24, 2019

@buchgr buchgr requested a review from ishikhman July 24, 2019 13:44
@buchgr buchgr requested a review from ola-rozenfeld as a code owner July 24, 2019 13:44
@jin jin added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Jul 25, 2019
buchgr added 3 commits July 29, 2019 13:23
make a drive by change of lib/remote:srcs collecting all the :srcs
filegroups of its subpackages
@buchgr buchgr force-pushed the refactor-blobstore branch from c270fee to ac28091 Compare July 29, 2019 11:57
@buchgr
Copy link
Copy Markdown
Contributor Author

buchgr commented Jul 29, 2019

each commit can be reviewed independently.

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

@borkaehw
Copy link
Copy Markdown

I like the refactoring, but there is one unsolved issue. If we are going to make another remote/grpc package for GprcBlobStore.java, there will still be circular dependencies.

In short

  • ByteStreamUploader.java
  • CacheNotFoundException.java
  • Chunker.java
  • ExecutionStatusException.java
  • ReferenceCountedChannel.java
  • RemoteRetrier.java
  • RemoteRetrierUtils.java
  • Retrier.java

and

  • SimpleBlobStoreFactory.java

can't be in the same package.

It makes sense to leave the first part in remote. How about also moving SimpleBlobStoreFactory.java into remote/common? And perhaps SimpleBlobStoreActionCache.java can also go into remote/common.

@bazel-io bazel-io closed this in 797f292 Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes team-Remote-Exec Issues and PRs for the Execution (Remote) team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants