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/01 04:46:00 UTC
[kudu-CR] Reduce arguments in file Read API
Grant Henke has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6770
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can\u2019t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
27 files changed, 154 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/1
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>
[kudu-CR] Reduce arguments in file Read API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 278: req, offset));
> Ah, missed that. Yeah, let's keep the original offset around and incorporat
Done
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 183: Status NonShortRead(T* file, uint64_t offset, Slice* result);
> Do we still need the templated Read() though? Can't we just call reader->Re
I am not strong on c++ templates. However, I think we need to provide some specialized "ReadableFileType" implementation for RandomAccessFile and RWFile since they don't share a common interface for the Read() method. The API just happens to be the same.
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h
File src/kudu/util/env.h:
PS3, Line 355: virtual Status Read(Slice* result)
> Hmm... I am not sure I understand the purpose of changing the function sign
I think the primary motivation for this refactor was the new ReadV() API that Grant is working on. ReadV() operates on multiple caller-provided buffers, which means a list of (scratch, length, result slice) tuples. It's tough to reason about ReadV()'s IN and OUT parameter semantics since you have to consider both these tuples and the list of tuples itself. On the other hand, I think an IN/OUT list of IN/OUT slices is a little simpler.
Outside of ReadV(), doing away with short reads means there is only one case where a Slice member is updated by Read(), and that's here in SequentialFile (since a need to deal with EOF). In all other cases, the Slice _buffer_ is written to, but the Slice _members_ aren't changed at all. So Slice acts more like an IN parameter than a true IN/OUT one.
FWIW, memory-mapped file reading was removed Dec 2015 (commit a26e074), so resetting a Slice for zero-copy is no longer a concern.
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 183: Status Read<RandomAccessFile>(RandomAccessFile* file, uint64_t offset, Slice* result) {
> I am not strong on c++ templates. However, I think we need to provide some
Hmm, I think you can get away with this:
template<typename T>
Status Read(T* file, uint64_t offset, Slice* result) {
return file->Read(offset, result);
}
You may need to add explicit specialization (see tools/data_gen_util.cc for an example), but I don't think so because all of the callers to Read() are within this compilation unit.
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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
Gerrit-HasComments: Yes
[kudu-CR] Reduce arguments in file Read 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/6770
to look at the new patch set (#6).
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can’t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 157 insertions(+), 202 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/6
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h
File src/kudu/util/env.h:
Line 355: virtual Status Read(Slice* result) = 0;
> I think the primary motivation for this refactor was the new ReadV() API th
Before the patch that made the read calls "read fully" there was a length IN and a length OUT parameter. The length OUT parameter was in the OUT slice. Now they are essentially the same.
Currently the Read() API essentially requires 3 parameters:
- offset (IN): position to start reading from
- length (IN/OUT): the amount of data to read
- scratch (OUT): memory to read into
The ReadV() API will similarly need:
- offset (IN): position to start reading from
- results (IN/OUT): A vector of:
- length (IN): the amount of data to read
- scratch (OUT): memory to read into
Since Slice is commonly used today to encapsulate a scratch buffer and define it's relevant length I thought it would work as an efficent IN/OUT parameter for both READ() and READV().
This results in the following API:
- Status Read(uint64_t offset, Slice* result)
- Status ReadV(uint64_t offset, vector<Slice>* results)
> in the case where we might be reading from a memory-mapped file, we wouldn't necessarily want to use the buffer referenced in result.data; we would likely want to reset the Slice to reference mapped memory for zero-copy.
Yes this wouldn't necessarily work well for that. In that case a scratch buffer isn't needed at all. It doesn't look like this API handled that use case well in general since the Slice didn't "own" the data and required a memcpy to make sure the data stuck around.
Line 376: // Read up to "result.size" from the file starting at "offset".
> Why "up to"? Isn't guaranteed to read exactly that amount, or fail?
Technically if it failed with an IO error after a short read the scratch buffer would contain a partial result. Though the error status would indicated the result is invalid.
Line 382: // possible to read exactly 'length' bytes, an IOError is returned.
> This note should probably exist in the other Read() implementations too. Or
It's it on all the "bottom" level reads that any other read wraps. Do you think that is enough?
Line 504: // Read up to "result.size" from the file starting at "offset".
> Same question about "up to".
See earlier response.
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 278: req, offset));
> Nit: I think result->length() is actually more correct here. Yes, the EOF o
I think I agree. But the offset is being edited in the loop too. I would need to keep and reference the original offset too. Should I do that?
Line 308: *result = Slice(result->mutable_data(), r);
> Wouldn't result->truncate(r) be simpler?
Good idea. Done.
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util-internal.cc
File src/kudu/util/pb_util-internal.cc:
Line 51: }
> I don't think this is possible anymore, is it?
Done
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 183: Status NonShortRead(T* file, uint64_t offset, Slice* result);
> Is the indirection via NonShortRead() still needed? Or can we just call rea
Done
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/6/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 180: Status Read(T* file, uint64_t offset, Slice* result) {
> Dan helpfully refreshed my memory on how templating works, but then I forgo
Interesting. I thought I tried that initially. I must have had some detail wrong.
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h
File src/kudu/util/env.h:
Line 376: // Read up to "result.size" from the file starting at "offset".
Why "up to"? Isn't guaranteed to read exactly that amount, or fail?
PS3, Line 380: // This method will internally retry on EINTR and "short reads" in order to
: // fully read the requested number of bytes. In the event that it is not
: // possible to read exactly 'length' bytes, an IOError is returned.
This note should probably exist in the other Read() implementations too. Or (if you don't want to copy it repeatedly) it can be generalized somewhere and then linked to from the various Read() implementations.
PS3, Line 504: up to
Same question about "up to".
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 278: req, offset));
Nit: I think result->length() is actually more correct here. Yes, the EOF occurred on one of potentially several pread() calls, but the caller doesn't know how many calls were made, and so the caller would expect the error to describe the logical read itself.
Line 308: *result = Slice(result->mutable_data(), r);
Wouldn't result->truncate(r) be simpler?
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util-internal.cc
File src/kudu/util/pb_util-internal.cc:
PS3, Line 49: if (result.data() != buffer_.get()) {
: memcpy(buffer_.get(), result.data(), result.size());
: }
I don't think this is possible anymore, is it?
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 183: Status NonShortRead(T* file, uint64_t offset, Slice* result);
Is the indirection via NonShortRead() still needed? Or can we just call reader->Read() directly in ValidateAndReadData()?
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 183: Status Read<RandomAccessFile>(RandomAccessFile* file, uint64_t offset, Slice* result) {
> Done
Do we still need the templated Read() though? Can't we just call reader->Read(...) on L219 in ValidateAndReadData()?
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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
Gerrit-HasComments: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 183: Status NonShortRead(T* file, uint64_t offset, Slice* result);
> Hmm, I think you can get away with this:
Done
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/6/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
Line 180: Status Read(T* file, uint64_t offset, Slice* result) {
Dan helpfully refreshed my memory on how templating works, but then I forgot to update my comment from the last round. Sorry about that.
You should be able to do away with this function altogether and call reader->Read(*offset, &s) directly in ValidateAndReadData().
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h
File src/kudu/util/env.h:
Line 382: // possible to read exactly 'length' bytes, an IOError is returned.
> It's it on all the "bottom" level reads that any other read wraps. Do you t
I was thinking of SequentialFile, but then I remembered that its implementation is different (it uses fread_unlocked) so it probably doesn't make sense. Nevermind.
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 278: req, offset));
> I think I agree. But the offset is being edited in the loop too. I would ne
Ah, missed that. Yeah, let's keep the original offset around and incorporate it here. It's cheap.
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h
File src/kudu/util/env.h:
PS3, Line 355: virtual Status Read(Slice* result)
> I think the primary motivation for this refactor was the new ReadV() API th
Ah, I didn't know we actually deleted the memory mapping code. I just thought we switched away from it in the block manager implementation.
If this makes ReadV() easier to implement, I guess it makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Reduce arguments in file Read 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/6770
to look at the new patch set (#4).
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can’t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 160 insertions(+), 197 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/4
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read 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/6770
to look at the new patch set (#3).
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can\u2019t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 158 insertions(+), 183 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/3
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Reduce arguments in file Read 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/6770
to look at the new patch set (#7).
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can’t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 155 insertions(+), 205 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/7
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read 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/6770
to look at the new patch set (#5).
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can’t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 159 insertions(+), 201 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/5
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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] Reduce arguments in file Read 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/6770
to look at the new patch set (#2).
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can\u2019t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 158 insertions(+), 190 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/6770/2
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Reduce arguments in file Read API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Reduce arguments in file Read API
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h
File src/kudu/util/env.h:
PS3, Line 355: virtual Status Read(Slice* result)
Hmm... I am not sure I understand the purpose of changing the function signatures. That part of this patch doesn't seem like an improvement to me. Since we don't use Slice as an input-output parameter anywhere else in the codebase AFAIK I find this API a little surprising; I would have to open the header file to understand how it works when reading code.
It also requires additional setup by the user to put something into the Slice before passing it in, and in the case where we might be reading from a memory-mapped file, we wouldn't necessarily want to use the buffer referenced in result.data; we would likely want to reset the Slice to reference mapped memory for zero-copy. Of course, we can still do that with this API but providing scratch as an optional buffer to use and making Slice an output-only parameter makes that contract more explicit.
Same elsewhere.
If we want to change the signature of these functions, I'd just advocate for putting scratch before slice in the argument order to conform with the style guide, since scratch is really an input-output parameter (arguably even just an input parameter).
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Reduce arguments in file Read API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: Reduce arguments in file Read API
......................................................................
Reduce arguments in file Read API
Because reads are now always read fully or result in an error if
all bytes can’t be read, we can simplify the Read API to pass
only a Slice instead of a slice, scratch, and length.
This patch changes the file Read APIs and all similar API calls
that call it.
Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
Reviewed-on: http://gerrit.cloudera.org:8080/6770
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/cfile/cfile_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-stress-test.cc
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/fs_manager-test.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_copy_source_session.h
M src/kudu/util/env-test.cc
M src/kudu/util/env.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/pb_util-internal.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/rolling_log.cc
28 files changed, 155 insertions(+), 205 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4
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