David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 35: (12 comments) posting my previous comments. looking through the rest now http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: PS35, Line 186: of 5000. nit: "after 5000 runs" PS35, Line 205: disks data directories? PS35, Line 226: void BlockManagerTest<LogBlockManager>::RunBlockDistributionTest(const vector<string>& paths) { could you merge this method with the method above? a lot of the code seems pretty common. What if it had a signature like: void BlockManagerTest::DoRunBlockDistributionTest(const vector<string>& paths, vector<int> per_path_write_count, vector<int>* num_files_per_path) PS35, Line 227: const char* kTestData = "test data"; pull this somewhere common? PS35, Line 228: files_in_each_path these files are "block_containers" right? PS35, Line 230: for (int d: { 1, 5 }) { why not 3 in this case? PS35, Line 237: ScopedWritableBlockCloser closer; why not use this in the fbm too? PS35, Line 344: ASSERT_EQ(paths.size() * 7, sum); nit: this is not quite the same assertion as before. any special reason to change it? PS35, Line 447: // Store the DataDirGroupPB for tests that reopen the block manager. : CHECK(this->bm_->dd_manager()->GetDataDirGroupPBForTablet( : this->test_tablet_name_, &this->test_group_pb_)); this seems sketchy. are you reusing state across tests? what if I only run the other tests? PS35, Line 678: true nit: add /* bool_arg_name */ PS35, Line 692: true, same http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS35, Line 170: next remove -- To view, visit http://gerrit.cloudera.org:8080/6636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b Gerrit-PatchSet: 35 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
