Adar Dembo has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy ......................................................................
Patch Set 1: (8 comments) Mike should probably review this too. http://gerrit.cloudera.org:8080/#/c/7701/1//COMMIT_MSG Commit Message: PS1, Line 9: with Nit: into PS1, Line 11: table tablet PS1, Line 11: sync synced PS1, Line 13: I did a manual test to copy tablet with size of ~37GB. : With this change, the operation time dropped down from ~718s : to ~523s. We talked about this in person a little bit, but could you provide more details about the testing set up? Please specify how many tservers were involved. If just one, specify the OS, the filesystem, the disk setup, etc. http://gerrit.cloudera.org:8080/#/c/7701/1/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: Line 196: TEST_F(TabletCopyClientTest, TestDownloadAllBlocks) { Maybe as part of this test we could verify that the number of fsyncs due to block downloading was some number we expect? We don't currently have metrics for fsyncs but that's something we could feasibly add (to the LogBlockManager or perhaps even to the file implementations in env_posix.cc). http://gerrit.cloudera.org:8080/#/c/7701/1/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: Line 27: #include "kudu/fs/block_manager.h" Why do you need this? Can't you forward declare BlockTransaction? PS1, Line 164: belong belonging Line 177: // Data block is opened with options so that it will fsync() on close. Update this (and maybe the entire comment). -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
