refactor the cache.Cache interface #162
Conversation
|
Please let me know when it's ready for review. |
mostynb
force-pushed
the
cache_interface_refactor
branch
4 times, most recently
from
54dd298
to
e7fc03b
Compare
|
@buchgr: I think this is ready for review. I will try to stress test it some more today. |
mostynb
changed the title
|
Seems to WFM building a large project like tensorflow. |
|
Did you get a chance to test the S3 and GCS implementations? If you haven't I can test the GCS implementation but I don't have an AWS account. |
|
I nave neither AWS nor GCS accounts to test- if you could test GCS that would be great. I started working on some s3 tests with github.com/johannesboyne/gofakes3, but it seems to be broken with the minio-go client (if I Put and then Get an object, the bytes come back wrapped with some sort of signature-chunk info). I'll try setting up a minio server and do a full system test with it. |
|
I ll give GCS a try and report back. |
|
Pushed a couple of fixes and some log format changes, after testing with minio. Building bazel-remote itself with a bazel-remote + minio works pretty well with gRPC, but it's quite a bit slower using HTTP. I need to investigate the difference. |
|
Tested this quickly with gRPC and S3 and works fine. |
Background: Lukas was expecting cache hits from the proxy backend after removing the bazel-remote cache and restarting. This was fixed in this PR since it also proxies HEAD/Contains operations. |
|
I just tested this patch with GCS. Seems to work great too! |
|
Re the performance difference between using http and grpc between bazel and the cache, it seems to be unrelated to this change. I think it's just that uploading go stdlib (lots of small files) is much less efficient with http than with grpc: So I will rebase and land this if there are no objections. |
I am giving it a review right now. 1sec please :-).
I believe the reason for that is different. The gRPC implementation in Bazel uploads all outputs of an action concurrently while the HTTP implementation does so serially. This can be fixed in Bazel. |
mostynb
force-pushed
the
cache_interface_refactor
branch
from
a3a5146
to
7702c5f
Compare
Yes, of course.
I figured that must be the case. |
|
LGTM. Thanks @mostynb |
Since all the proxy backends use the same copy+pasted logic to use the disk cache, refactor to put that functionality in the disk cache. This simplifies the proxy backends. Contains is now also proxied to the backend, instead of only returning whether or not the local disk cache contains the item. Fixes buchgr#161.
mostynb
force-pushed
the
cache_interface_refactor
branch
from
1b4a136
to
c3b9160
Compare
Confirm that we can fill the proxy backend via the disk cache, and vice versa. Followup to buchgr#162.
Confirm that we can fill the proxy backend via the disk cache, and vice versa. Followup to buchgr#162.
Confirm that we can fill the proxy backend via the disk cache, and vice versa. Followup to buchgr#162.
Confirm that we can fill the proxy backend via the disk cache, and vice versa. Followup to buchgr#162.
Confirm that we can fill the proxy backend via the disk cache, and vice versa. Followup to #162.
Since all the proxy backends use the same copy+pasted logic to use
the disk cache, refactor to put that functionality in the disk cache.
This simplifies the proxy backends.
Fixes #161.