David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1970: node density integration test ......................................................................
Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/6662/9/src/kudu/integration-tests/dense_node-itest.cc File src/kudu/integration-tests/dense_node-itest.cc: PS9, Line 68: DenseNodeTest in general would it make sense to have this test be parameterized and also run with fsync enabled? PS9, Line 71: DenseNodeTest an intro to what this test targets here or in the class header would be nice PS9, Line 71: DenseNodeTest also this test seems to be more of a benchmark that an test. change the name? PS9, Line 100: // Inject steroids into the MM. hum 100 seems excessive an unrealistic even for denser nodes, maybe 10 would be more acceptable? PS9, Line 131: for (int i = 1; i < FLAGS_num_columns; i++) { : b.AddColumn(Substitute("i$0", i))->Type(KuduColumnSchema::INT32)->NotNull(); : } would it make sense to also have larger string columns? seems like this will build very symmetric rowsets whereas assymetric ones where string columns dominate the size of a rowset are common PS9, Line 145: us nit: s/us/the test http://gerrit.cloudera.org:8080/#/c/6662/9/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: PS9, Line 141: starting what is "starting" here? writing the instance file? booting up everything? -- To view, visit http://gerrit.cloudera.org:8080/6662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie9b5d01557eb41d386ce92f576ed01ec658e8e7d Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
