define cache for seed file in seed pattern #1305

wangforthinker · 2020-04-27T06:50:00Z

Signed-off-by: allen.wq [email protected]

Ⅰ. Describe what this PR did

Add implementation for seed cache. Please see refer #1284.

Ⅱ. Does this pull request fix one issue?

NONE.

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

Add unit test.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

pouchrobot · 2020-04-27T06:50:02Z

Thanks for your contribution. 🍻 @wangforthinker
While we thought PR TITLE could be more specific, longer than 20 chars.
Please edit this PR title instead of opening a new one.
More details, please refer to https://github.com/dragonflyoss/Dragonfly/blob/master/CONTRIBUTING.md

codecov-io · 2020-04-27T06:53:36Z

Codecov Report

Merging #1305 into master will increase coverage by 0.56%.
The diff coverage is 73.23%.

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
+ Coverage   51.20%   51.76%   +0.56%     
==========================================
  Files         124      125       +1     
  Lines        8152     8365     +213     
==========================================
+ Hits         4174     4330     +156     
- Misses       3639     3674      +35     
- Partials      339      361      +22

Impacted Files	Coverage Δ
pkg/bitmap/bitmap.go	`91.15% <0.00%> (-5.95%)`	⬇️
dfdaemon/seed/cache.go	`76.47% <76.47%> (ø)`

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 734cde5...0aac763. Read the comment docs.

hhhhsdxxxx · 2020-04-27T06:54:15Z

lgtm!
Reviewed-by [email protected]

xujihui1985 · 2020-04-27T06:57:58Z

lgtm
Reviewed-by [email protected]

lowzj · 2020-04-27T08:03:26Z

dfdaemon/seed/cache.go

+			sizeOf64Bits++
+		}
+
+		fcb.blockMeta, err = bitmap.NewBitMap(sizeOf64Bits, false)


It's a little wired to calculate the sizeOf64Bits. Actually, the sizeOf64Bits is part of the bitmap's concrete implementation and the caller doesn't care about it when using the bitmap. Could we just directly provide a constructor with the argument numberOfBits? WDYT.

lowzj · 2020-04-27T08:18:38Z

dfdaemon/seed/cache.go

+
+	fcb := &fileCacheBuffer{path: path, fw: fw, fullSize: fullSize, memoryCache: memoryCache}
+	if memoryCache {
+		fcb.blockOrder = blockOrder


I remember that the blockOrder is in range [10,31], should it check here?

In this object, the range of blockOrder is not concerned. And for the convenience of unit test, blockOrder may less than 10.

lowzj · 2020-04-27T08:22:03Z

dfdaemon/seed/cache.go

+// Close closes the file writer.
+func (fcb *fileCacheBuffer) Close() error {
+	if err := fcb.Sync(); err != nil {
+		return err


Should it close the fw no matter whether the err is nil?

In some case, Close() may be called again if first close failed.

lowzj · 2020-04-27T08:36:01Z

dfdaemon/seed/cache.go

+		off = 0
+	}
+
+	// if size <= 0, set range to [off, fullSize-1]


Is there any consideration about supporting the case of empty files or data? HTTP Range request doesn't support empty files.

In the case, if size <=0, it set range from off to the end of file. Not empty data.

So it means that the empty data is not supported here.

lowzj · 2020-04-27T08:50:54Z

dfdaemon/seed/cache.go

+	offArr := []int64{}
+	bufArr := []*bytes.Buffer{}
+
+	fcb.RLock()


The Lock operation in this method and syncMemoryCache cannot prevent concurrency writing problem.

the elem of memCacheMap represents a buffer cache, if buffer cache stores in memCacheMap, its content could not be changed util buffer gc.
So the Lock aims to protect the memCacheMap not the buffer content.

lowzj · 2020-04-27T08:55:05Z

dfdaemon/seed/cache.go

+	}
+	fcb.RUnlock()
+
+	for i := 0; i < len(offArr); i++ {


It's better to sort the offArr in order to merge and write blocks sequentially to improve the performance when the number of blocks is large.

lowzj · 2020-05-12T12:53:22Z

dfdaemon/seed/cache.go

+	})
+
+	for i := 0; i < len(offArr); i++ {
+		err = fcb.syncBlockToFile(bufArr[i].Bytes(), offArr[i])


It will introduce a bug here after sorting offArr only because that the element in offArr and bufArr with the same index are belonged to different data. Maybe the sort step should be removed if no other good solution.

Fix it by define struct {off int, buf *bytes.buffer} and sort.

Signed-off-by: allen.wq <[email protected]>

lowzj

LGTM

wangforthinker changed the title ~~add cache for seed~~ define cache for seed file in seed pattern Apr 27, 2020

pouchrobot added the size/XXL label Apr 27, 2020

lowzj reviewed Apr 27, 2020

View reviewed changes

weekly-digest bot mentioned this pull request May 3, 2020

Weekly Digest (26 April, 2020 - 3 May, 2020) #1323

Closed

wangforthinker force-pushed the feat/add-seed-cache-prepare-pr branch 4 times, most recently from 0aac763 to afa306e Compare May 12, 2020 12:37

lowzj reviewed May 12, 2020

View reviewed changes

wangforthinker force-pushed the feat/add-seed-cache-prepare-pr branch 2 times, most recently from 0e1f1bf to e09f4c8 Compare May 15, 2020 02:38

add cache for seed.

a2a6d82

Signed-off-by: allen.wq <[email protected]>

wangforthinker force-pushed the feat/add-seed-cache-prepare-pr branch from e09f4c8 to a2a6d82 Compare May 15, 2020 09:41

lowzj approved these changes May 15, 2020

View reviewed changes

lowzj merged commit f131b5f into dragonflyoss:master May 15, 2020

wangforthinker deleted the feat/add-seed-cache-prepare-pr branch May 27, 2020 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

define cache for seed file in seed pattern #1305

define cache for seed file in seed pattern #1305

wangforthinker commented Apr 27, 2020

pouchrobot commented Apr 27, 2020

codecov-io commented Apr 27, 2020 •

edited

Loading

hhhhsdxxxx commented Apr 27, 2020

xujihui1985 commented Apr 27, 2020

lowzj Apr 27, 2020 •

edited

Loading

wangforthinker May 11, 2020

lowzj Apr 27, 2020

wangforthinker May 9, 2020 •

edited

Loading

lowzj Apr 27, 2020

wangforthinker May 9, 2020

lowzj Apr 27, 2020

wangforthinker May 9, 2020

lowzj May 12, 2020

lowzj Apr 27, 2020

wangforthinker May 9, 2020

lowzj Apr 27, 2020

wangforthinker May 9, 2020

lowzj May 12, 2020

wangforthinker May 13, 2020 •

edited

Loading

lowzj left a comment

define cache for seed file in seed pattern #1305

define cache for seed file in seed pattern #1305

Conversation

wangforthinker commented Apr 27, 2020

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

pouchrobot commented Apr 27, 2020

codecov-io commented Apr 27, 2020 • edited Loading

Codecov Report

hhhhsdxxxx commented Apr 27, 2020

xujihui1985 commented Apr 27, 2020

lowzj Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangforthinker May 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangforthinker May 13, 2020 • edited Loading

Choose a reason for hiding this comment

lowzj left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 27, 2020 •

edited

Loading

lowzj Apr 27, 2020 •

edited

Loading

wangforthinker May 9, 2020 •

edited

Loading

wangforthinker May 13, 2020 •

edited

Loading