Conversation
raakella1
force-pushed
the
read_impl
branch
5 times, most recently
from
96c7fc7 to
5d979e7
Compare
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
===========================================
+ Coverage 61.68% 80.98% +19.29%
===========================================
Files 15 17 +2
Lines 462 999 +537
Branches 35 105 +70
===========================================
+ Hits 285 809 +524
+ Misses 158 140 -18
- Partials 19 50 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
raakella1
force-pushed
the
read_impl
branch
2 times, most recently
from
79c9071 to
b9be84e
Compare
src/lib/volume_mgr.cpp
Outdated
| blks_to_read.emplace_back(index_kvs[start_idx].first.key(), homestore::MultiBlkId(blk_num, blk_count, chunk_num)); | ||
| start_idx = i; | ||
| if(!is_contiguous) { | ||
| // this is the last entry in the index_kvs |
There was a problem hiding this comment.
is this correct? if is_contigous is false, it will also come here, it can be any non-1st (i != 0) entry in index_kvs, right? We have to go to next loop to keep accumulating the conginous ones.
src/lib/volume_mgr.cpp
Outdated
| continue; | ||
| } | ||
| } | ||
| // prepare the blkid for the read |
There was a problem hiding this comment.
I would suggest to:
if(is_contiguous && i < index_kvs.size() - 1) {
continue;
}
if (!is_contigous) {
// build build id and put back to result
} else {
assert ( i == index_kvs.size() -1);
// last blk id, push back to result
// fall through to let for loop to exit;
}
| TEST_F(VolumeIOTest, SingleVolumeReadData) { | ||
| // Write and verify fixed LBA range to single volume multiple times. | ||
| auto vol = volume_list().back(); | ||
| uint32_t nblks = 5000; |
There was a problem hiding this comment.
Can we add some io's which have hole and your read covers those ranges with holes and ranges with data this PR or another PR.
There was a problem hiding this comment.
I will have a separate PR just for adding more test cases
| sisl::sg_list data_sgs; | ||
| data_sgs.iovs.emplace_back(iovec{.iov_base = vol_req->buffer, .iov_len = data_size}); | ||
| data_sgs.size = data_size; | ||
| void HomeBlocksImpl::generate_blkids_to_read(const index_kv_list_t& index_kvs, read_blks_list_t& blks_to_read) { |
There was a problem hiding this comment.
Please write a unit test case for this API alone. We can fake input of index_kvs, and assert on output blks_to_read.
You can do it on next PR.
Unit test coverage can be:
- all contigous.
- all dis-contigous.
- a bunch of mixed ones.
There was a problem hiding this comment.
I will have a separate PR to cover different cases for the holes
4 participants
No description provided.