Alexey Serbin has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 35: (28 comments) http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/cfile/bloomfile-test-base.h File src/kudu/cfile/bloomfile-test-base.h: PS35, Line 74: fs::CreateBlockOptions() nit: would just '{}' be enough? http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: PS35, Line 218: CreateBlockOptions() nit here and below: would '{}' fit in here? http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: PS35, Line 260: this-> is 'this' necessary here? 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 137: Status nit: consider adding 'static' specifier -- as I see this method does not depend on the state of the object. PS35, Line 155: &file_count nit: why not to pass 'num_files' itself as an argument? http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: Line 17: nit: please include at least <string> and <vector> since they are used in the code. Line 18: #include "kudu/fs/fs.pb.h" nit: please include <gtest/gtest.h> and <gflags/gflags_declare.h> PS35, Line 55: CHECK_OK nit: why not just ASSERT_OK() ? PS35, Line 56: CHECK_OK ditto PS35, Line 69: string test_tablet_name_; nit: I don't see it's changing during the tests, so consider adding 'const'. PS35, Line 70: CreateBlockOptions test_block_opts_ const? PS35, Line 72: DataDirGroupPB pb_; nit: is it really needed as a member? Would local variable be enough where needed? PS35, Line 80: ASSERT_TRUE(s.IsNotFound()); Here and everywhere in the tests consider adding s.ToString() in case if it fails, that helps a lot other people during troubleshooting if this ever fails: ASSERT_TRUE(s.IsNotFound()) << s.ToString(); PS35, Line 96: for (int i = 0; i < pb.uuids().size(); i++) { Would it make sense to compare pb.uuids.size() and pb_.uuids.size() prior to comparing elements in this for() loop? PS35, Line 102: dd Does it make sense to check for invariants on 'dd' after this call? What are the expected side-effects after successful call of GetNextDataDir() for 'dd'? PS35, Line 120: DataDir* dd; : ASSERT_OK(dd_manager_->GetNextDataDir(test_block_opts_, &dd)); : dd_manager_->DeleteDataDirGroup(test_tablet_name_); : Status s = dd_manager_->GetNextDataDir(test_block_opts_, &dd); Is there anything specific for 'dd' in the course of there calls? If yes, consider adding corresponding assertions. PS35, Line 129: FLAGS_fs_data_dirs_full_disk_cache_seconds = 0; If changing this flag on-the-fly, does it make sense to add the 'runtime' tag for the flag? PS35, Line 235: FindOrDie nit: since this is a test, it's possible to use something like ... x = FindOrNull(...); ASSERT_NE(nullptr, x); http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS35, Line 449: FLAGS_fs_target_data_dirs_per_tablet A paranoid nit: what if FLAGS_fs_target_data_dirs_per_tablet is set to 2^31 ? Consider either adding a validator for the flag or using unsigned int for group_target_size or enforcing consistency by some other means. http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS35, Line 87: uuid_indices nit: consider adding 'const' specified for this method. PS35, Line 225: void nit: would the caller ever be interested to know if any group were effectively deleted or not? http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: PS35, Line 93: string tablet_id const string& tablet_id ? Line 112: string tablet_id = rowset_metadata_->tablet_metadata()->tablet_id(); ditto PS35, Line 127: string tablet_id ditto http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/tablet/multi_column_writer.cc File src/kudu/tablet/multi_column_writer.cc: Line 52: CreateBlockOptions block_opts({ tablet_id_ }); nit: const? http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/tablet/multi_column_writer.h File src/kudu/tablet/multi_column_writer.h: PS35, Line 20: #include <glog/logging.h> nit: move this after the std headers. PS35, Line 90: std::string tablet_id_; nit: const? http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: PS35, Line 629: GetDataDirGroupPBForTablet Is it worth to check for the return value of GetDataDirGroupPBForTablet()? -- 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
