Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9037 )
Change subject: heavy-update-compaction-itest ...................................................................... Patch Set 1: (5 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 st Done http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@168 PS1, Line 168: 100 rows. > nit: might not be 100, right? Done http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@217 PS1, Line 217: scanner.SetTimeoutMillis(120 * 1000); > make this a flag so that we can change this if we change "rows" or "rounds" Done 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& Done 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 o I don't consider this to be a fatal failure, since the test can keep going if it fails without causing a logic or memory-safety error. The upside is that if this case were to fail it may be easier to debug if you get the full set of errors instead of short circuiting. -- 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 20:14:38 +0000 Gerrit-HasComments: Yes
