Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9037 )
Change subject: heavy-update-compaction-itest ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc File src/kudu/integration-tests/heavy-update-compaction-itest.cc: http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@92 PS1, Line 92: b.AddColumn("val_a")->Type(KuduColumnSchema::STRING)->NotNull(); Maybe parameterize (or at least store in a class constant) the number of string rows? Would also help avoid the "i <= 5" magic numbers below. http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@225 PS1, Line 225: const auto & Nit: const auto& http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@230 PS1, Line 230: EXPECT_EQ(actual_val, final_values[final_values_offset++]); Nit: just curious why you used EXPECT here and not ASSERT, given that the other checks in this test are all ASSERT? -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 16 Jan 2018 19:34:37 +0000 Gerrit-HasComments: Yes
