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/04 18:27:57 UTC

[kudu-CR] env: add WriteV() API

Grant Henke has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6800

Change subject: env: add WriteV() API
......................................................................

env: add WriteV() API

Adds WriteV() methods to RWFile and WritableFile that allows
writing multiple data Slices in one call. The implementation
leverages the pwritev system call when possible and simulates it
with pwrite calls when unavailable.

Additionally adds WriteV()/AppendV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support writing
checksums and block data in a single call.

Change-Id: I30acfa2e4918ef945c55646647913b36a07daaa4
---
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
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
10 files changed, 202 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/6800/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I30acfa2e4918ef945c55646647913b36a07daaa4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke <gr...@gmail.com>

[kudu-CR] env: add WriteV() 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/6800

to look at the new patch set (#2).

Change subject: env: add WriteV() API
......................................................................

env: add WriteV() API

Adds WriteV() methods to RWFile and WritableFile that allows
writing multiple data Slices in one call. The implementation
leverages the pwritev system call when possible and simulates it
with pwrite calls when unavailable.

Additionally adds WriteV()/AppendV() methods to the block manager abstraction.
These methods will be used in KUDU-463 to support writing
checksums and block data in a single call.

Change-Id: I30acfa2e4918ef945c55646647913b36a07daaa4
---
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
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
10 files changed, 202 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/6800/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I30acfa2e4918ef945c55646647913b36a07daaa4
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

[kudu-CR] env: add WriteV() API

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.

Grant Henke has posted comments on this change.

Change subject: env: add WriteV() API
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6800/1/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

Line 478: }
> Any particular reason to preserve this? It's a private function; if it's no
Done


Line 480: Status CFileWriter::WriteRawDataV(const vector<Slice> &data) {
> vector<Slice>&
Done


http://gerrit.cloudera.org:8080/#/c/6800/1/src/kudu/cfile/cfile_writer.h
File src/kudu/cfile/cfile_writer.h:

Line 189:   Status WriteRawDataV(const vector<Slice> &data);
> Should be vector<Slice>& (& next to the type).
Done


http://gerrit.cloudera.org:8080/#/c/6800/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 1063:   virtual Status AppendV(const std::vector<Slice>& data) OVERRIDE;
> Don't need std:: prefix here.
Done


http://gerrit.cloudera.org:8080/#/c/6800/1/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:

Line 163:     // Force short reads to half the slice length
> You mean writes?
Done


http://gerrit.cloudera.org:8080/#/c/6800/1/src/kudu/util/env.h
File src/kudu/util/env.h:

Line 448:   virtual Status AppendV(const std::vector<Slice> &data) = 0;
> std::vector<Slice>&
Done


Line 543:   virtual Status WriteV(uint64_t offset, const vector<Slice> &data) = 0;
> Need std:: prefix here. Also vector<Slice>&
Done


http://gerrit.cloudera.org:8080/#/c/6800/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 408:   // Convert the results into the iovec vector to request
> Nit: terminate comments that are English sentences with a period.
Done


Line 571:     Status s = DoWriteV(fd_, filename_, filesize_, data);
> Hmm, this seems weird; shouldn't we RETURN_NOT_OK() and not touch filesize_
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I30acfa2e4918ef945c55646647913b36a07daaa4
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: Tidy Bot
Gerrit-HasComments: Yes