New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix http cache miss for servers which supports etag only #179

Merged
merged 1 commit into from Oct 30, 2018

Conversation

Projects
None yet
5 participants
@silenceshell
Contributor

silenceshell commented Oct 26, 2018

Ⅰ. Describe what this PR did

I used harbor as our docker registry, with Dragonfly as a p2p proxy, and I found that supernode has never hit the cache in ./repo/download.

It seems that supernode always downloads the blob file again from harbor, even the same file has exist in the ./repo/download/{taskid}/files

In the method CacheDetectorImpl.parseBreakNum, supernode only uses the Last-Modified to check whether the cache has expired.

However, Harbor only returns the ETag header in the response message. The ETag is the sha256 digest of the blob.

This PR adds support for ETag identifier with http cache. parseBreakNum will return 0 when both Last-Modified and Etag check failed.

Ⅱ. Does this pull request fix one issue?

NA

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

NA

Ⅳ. Describe how to verify it

  1. use Harbor as docker registry(I don't think harbor is nessary, however I have not tried), and Dragonfly as a p2p proxy.
  2. docker pull a image from harbor on a node. check the app.log on supernode, will find that supernode download the blob file from harbor.
  3. wait 3-6 minites, check data-gc.log on supernode, when it says supernode has gc all count, docker pull the image again.
  4. check the app.log on supernode, will get cache full hit for taskId: .... That means the cache hits.

Ⅴ. Special notes for reviews

NA

@pouchrobot pouchrobot added the size/M label Oct 26, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Oct 26, 2018

Codecov Report

Merging #179 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #179   +/-   ##
=======================================
  Coverage   81.43%   81.43%           
=======================================
  Files          28       28           
  Lines        1282     1282           
=======================================
  Hits         1044     1044           
  Misses        207      207           
  Partials       31       31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae3a8a9...aaab306. Read the comment docs.

@allencloud allencloud added the kind/bug label Oct 26, 2018

@lowzj lowzj self-requested a review Oct 26, 2018

&& HttpClientUtil.isExpired(task.getSourceUrl(), fileMetaData.getLastModified(), task.getHeaders())) {
// both lastModified and eTag not satisfy
if (HttpClientUtil.isExpired(task.getSourceUrl(), fileMetaData.getLastModified(), task.getHeaders()) &&
HttpClientUtil.isNoneETagMatch(task.getSourceUrl(), fileMetaData.getETag(), task.getHeaders())) {

This comment has been minimized.

@lowzj

lowzj Oct 26, 2018

Member

It's better to check Last-Modified and ETag together by only sending one http request.

@silenceshell

This comment has been minimized.

Contributor

silenceshell commented Oct 29, 2018

@lowzj Thanks for your review. I have updated the code, now only send one request to check. Would you please help to review again?

@lowzj

The wercker/build pipeline is failed, you could rebase from master and push again.

@@ -231,6 +231,7 @@ private HttpURLConnection createConn(String taskId, String fileUrl, String[] hea
}
if (code == checkCode) {
fileMetaDataService.updateLastModified(taskId, conn.getLastModified());
fileMetaDataService.updateETag(taskId, conn.getHeaderField("ETag"));

This comment has been minimized.

@lowzj

lowzj Oct 30, 2018

Member

This could be set together with lastModified because of they are used together to do the same thing always.

This comment has been minimized.

@silenceshell

silenceshell Oct 30, 2018

Contributor

Thanks, this is much better. I have updated the code, would you please review again?

@silenceshell silenceshell force-pushed the silenceshell:master branch from f72b231 to 2382878 Oct 30, 2018

@pouchrobot pouchrobot added size/S and removed size/M labels Oct 30, 2018

fix http cache miss for servers which supports etag only
check Last-Modified and ETag together

Signed-off-by: silenceshell <me@ieevee.com>

as

@silenceshell silenceshell force-pushed the silenceshell:master branch from b9713b2 to aaab306 Oct 30, 2018

@lowzj

lowzj approved these changes Oct 30, 2018

@lowzj

This comment has been minimized.

Member

lowzj commented Oct 30, 2018

lgtm

@lowzj lowzj merged commit 66abe15 into dragonflyoss:master Oct 30, 2018

5 checks passed

ci/circleci: markdownlint-misspell Your tests passed on CircleCI!
Details
ci/circleci: unit-test-golang Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
wercker/build Wercker pipeline passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment