Conversation
|
Adding the exact local benchmark method and temporary harness used for the current numbers on the latest pushed branch state. Method:
Commands: cd /tmp/juicefs-bench-main
go run ./.tmp_clonebench -redis-addr 127.0.0.1:6379 -db 10 -files 1000 -concurrency 4 -run-id baseline_1000_clean
cd /Users/vyalamar/Documents/juicefs
go run ./.tmp_clonebench -redis-addr 127.0.0.1:6379 -db 10 -files 1000 -concurrency 4 -run-id pr_1000_cleanResults:
|
|
These tests are passing. ``bash |
|
@zhijian-pro @eakman-datadog please help review. |
There was a problem hiding this comment.
Pull request overview
Adds a Redis backend implementation of doBatchClone so recursive clone traversal can clone non-directory entries in bounded batches using pipelined reads and transactional batched writes, matching the SQL backend’s batching behavior and reducing per-entry Redis transaction overhead.
Changes:
- Implement
(*redisMeta).doBatchClonewith pipelined source reads and transactional batched destination writes, including aggregated slice refcount updates. - Add Redis-specific batch-clone tests covering shared chunk refs, mixed files/symlinks, space accounting, multi-chunk files, and partial-failure behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/meta/redis.go | Implements Redis doBatchClone batching logic (read pipeline + TxPipelined writes + sliceRef aggregation). |
| pkg/meta/redis_batchclone_test.go | Adds targeted tests for Redis batch clone correctness and edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
@jiefenghuang please help review. |
|
@jiefenghuang can you please help review. |
| if n == "." || n == ".." { | ||
| continue | ||
| } | ||
| if n == "file_A" || n == "file_B" { |
There was a problem hiding this comment.
Not major, but for good measure, I would remove this if-statement. It shouldn't bee needed with the skip on line 95.
| metaClient, err := newRedisMeta("redis", "127.0.0.1:6379/11", testConfig()) | ||
| if err != nil { | ||
| t.Fatalf("create meta: %v", err) | ||
| } | ||
| m, ok := metaClient.(*redisMeta) | ||
| if !ok { | ||
| t.Fatalf("expected *redisMeta, got %T", metaClient) | ||
| } | ||
| defer m.Shutdown() | ||
|
|
||
| if err := m.Reset(); err != nil { | ||
| t.Fatalf("reset meta: %v", err) | ||
| } | ||
| if err := m.Init(testFormat(), true); err != nil { | ||
| t.Fatalf("init meta: %v", err) | ||
| } |
There was a problem hiding this comment.
Not major. For readability and for easier reuse, I would shove this stuff in a helper function assuming it's the same for each test function--looked like it was.
eakman-datadog
left a comment
There was a problem hiding this comment.
@vyalamar did a first pass. Looks great! A few very minor comments. Going to take a second pass most likely tomorrow. But looks good so far.
|
@vyalamar Sorry for the slow response. I’ll review it in the next few days. |
sure Thank you. |
Summary
Implement Redis
doBatchClonefor recursive clone traversal.The SQL backend already batches non-directory clones. This change adds the corresponding Redis path so recursive clone can process files and symlinks in bounded sub-batches instead of falling back to per-entry Redis transactions for every non-directory child.
Behavior
sliceRefincrements by slice keyENOTSUPfor unsupported or uncertain cases so the caller can safely fall back to the existing per-entry pathNotes
nlink = 1, matching existing clone behaviorENOTSUPTests
go test -run TestRedisBatchClone ./pkg/meta -count=1Added coverage for:
Benchmark
I posted the local benchmark method, commands, and raw output in a follow-up PR comment.