You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2017/05/02 16:01:58 UTC
[kudu-CR] env: add ReadV() API
Grant Henke has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6779
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 292 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/1
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Reviewed-on: http://gerrit.cloudera.org:8080/6779
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 326 insertions(+), 15 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 312: Status DoReadV(int fd, const string& filename, uint64_t offset, vector<Slice>* results) {
> Yeah, thats a good idea. I think I can convert to an iov and use/adjust it
Right, you'll need to either consume an entire iov by shifting the iov array base/iov array size, or consume it partially by shifting iov.base/iov.size.
I omitted the second part because I figured it's not new; it's equivalent to the next_result Slice arithmetic you're already doing (as you pointed out).
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6779
to look at the new patch set (#3).
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 298 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/3
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6779
to look at the new patch set (#2).
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 298 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/2
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 3:
I need to add handling in the case the passed in vector exceeds IOV_MAX yet.
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6779
to look at the new patch set (#6).
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 331 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/6
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
Line 152: // Reads exactly the "results" aggregate bytes, based on each Slices "size",
> Slice's
Done
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env.h
File src/kudu/util/env.h:
Line 387: // Reads up to the "results" aggregate size, based on each Slices "size",
> Slice's
Done
Line 527: // Reads up to the "results" aggregate size, based on each Slices "size",
> Slice's
Done
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 312: Status DoReadV(int fd, const string& filename, uint64_t offset, vector<Slice>* results) {
> One other note: iovs from the iov array are "consumed" from front to back,
Yeah, thats a good idea. I think I can convert to an iov and use/adjust it for all the requests. I don't think its as simple as the preadv psuedo code you gave though.
If I understand preadv correctly, the number of bytes read could be a partial within a single iovec. That means I can't just shift by completed iovec's. Instead I need to shift the buffer and length within a partial iovec. This is what I was doing in the slice with:
Slice next_result(result.mutable_data() + bytes_rem, result.size() - bytes_rem);
Line 328: transform(wip_results.begin(), wip_results.end(), iov, [&](Slice curr) {
> I think ref-based iteration (Slice&) is acceptable here, as you did on L357
Done
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6779/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 330: Slice result = results->at(i);
> You can also do:
That doesn't seam to work. I can do the following though:
Slice result = (*results)[i];
Line 335: uint64_t completed_iov = 0;
> This could be an int (or size_t), since it's just the number of iovs.
Done
Line 363: for (size_t i = completed_iov; i < iov_size; i++) {
> Just to be safe, your injected short read test covers both of these cases,
Yeah, I inject a short read part way through the second of 2 Slices. This results in a full 5 bytes read in the first slice and 2 of 5 bytes read in the second.
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6779
to look at the new patch set (#7).
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 326 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/7
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6779/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 330: Slice result = results->at(i);
You can also do:
Slice& result = results[i];
Probably more idiomatic that way.
Line 335: uint64_t completed_iov = 0;
This could be an int (or size_t), since it's just the number of iovs.
Or will that mess with the "iov + completed_iov" arithmetic?
Line 363: for (size_t i = completed_iov; i < iov_size; i++) {
Just to be safe, your injected short read test covers both of these cases, right?
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6779
to look at the new patch set (#4).
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 295 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/4
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6779/6/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:
PS6, Line 475: WriteTestFile(env, kTestPath, data_size);
: ASSERT_NO_FATAL_FAILURE();
You can combine this:
NO_FATALS(WriteTestFile(...));
Line 487: results.push_back(Slice(scratch + shift, slice_size));
You can do:
results.emplace_back({scratch + shift, slice_size});
Or maybe it's without the braces; I can't remember.
PS6, Line 496: // Turn short reads off again
: FLAGS_env_inject_short_read_bytes = 0;
If the goal here is to reset the gflag for the next test, you don't need to worry about it. Every KuduTest defines a google::FlagSaver which restores all gflags to their default values when the test ends.
http://gerrit.cloudera.org:8080/#/c/6779/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 330: uint64_t cur_offset = offset;
> That doesn't seam to work. I can do the following though:
Right, I forgot that results is a pointer to a vector. Would this work?
Slice& result = (*results)[i];
This way you don't have to make a copy of the slice.
http://gerrit.cloudera.org:8080/#/c/6779/6/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 371: bytes_rem -= bytes_rem;
Once we hit this case, shouldn't we break out of the loop? If bytes_rem < iov[i].iov_len, that means bytes_rem will be 0 for every remaining iteration of the loop, and L369-371 will be no-ops.
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6779
to look at the new patch set (#5).
Change subject: env: add ReadV() API
......................................................................
env: add ReadV() API
Adds ReadV() methods to RWFile and RandomAccessFile that allows
reading data into multiple Slices in one call. The implementation
leverages the preadv system call when possible and simulates it
with pread calls when unavailable.
Additionally adds ReadV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support reading
checksums and block data in a single call.
Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs-test-util.h
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
9 files changed, 295 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/6779/5
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
PS1, Line 152: Slices
Slice's
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env.h
File src/kudu/util/env.h:
PS1, Line 387: Slices
Slice's
PS1, Line 527: Slices
Slice's
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 312: Status DoReadV(int fd, const string& filename, uint64_t offset, vector<Slice>* results) {
This implementation seems to be structured in the following way:
- vector<Slice> is the source of truth
- In a loop:
- Before the preadv(), convert vector<Slice> into iov array.
- After the preadv(), update vector<Slice> using results.
These vectors aren't long, but that's still a fair amount of copying and allocating. Instead, do you think it'd be possible to use the iov array as the source of truth? The number of elements in the array should only decrease with each preadv(), not increase, and you needn't redeclare it on the stack to do that; you can decrement iov_size to "remove" the last element in iov_array.
So the body would look something like this:
- Convert 'results' to iov array and iov_size up front.
- In a loop:
- Call preadv().
- Use preadv() results to change iov array entry base/bounds and iov_size.
PS1, Line 328: Slice
I think ref-based iteration (Slice&) is acceptable here, as you did on L357.
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6779/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 312: Status DoReadV(int fd, const string& filename, uint64_t offset, vector<Slice>* results) {
> This implementation seems to be structured in the following way:
One other note: iovs from the iov array are "consumed" from front to back, not back to front. So in addition to changing iov_size, you'll need to increment the pointer to the iov array itself. You could do that with some "iov_consumed" integer state and then the preadv() calls would look like this:
preadv(fd, iov + iov_consumed, iov_size - iov_consumed, cur_offset);
Just double check that "iov + iov_consumed" is correct; may also need to multiply iov_consumed by sizeof(iov).
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] env: add ReadV() API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: env: add ReadV() API
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6779/6/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:
Line 476: ASSERT_NO_FATAL_FAILURE();
> You can combine this:
Done
Line 487: results.push_back(Slice(scratch + shift, slice_size));
> You can do:
Done
Line 497: FLAGS_env_inject_short_read_bytes = 0;
> If the goal here is to reset the gflag for the next test, you don't need to
Done
http://gerrit.cloudera.org:8080/#/c/6779/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 330: uint64_t cur_offset = offset;
> Right, I forgot that results is a pointer to a vector. Would this work?
Done
http://gerrit.cloudera.org:8080/#/c/6779/6/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 371: bytes_rem -= bytes_rem;
> Once we hit this case, shouldn't we break out of the loop? If bytes_rem < i
Done
--
To view, visit http://gerrit.cloudera.org:8080/6779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8f7a62c8363b40baa064d9e63be1ece506f1e48
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes