Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool ...................................................................... Patch Set 11: (20 comments) http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@285 PS10, Line 285: : void RunActionS > Could you add 'stderr' and 'SCOPED_TRACE(stderr)' back in here? Like how it Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@394 PS10, Line 394: } : const string projection = JoinStrings(col_names, ","); : : v > For this sort of pattern, it's also possible to accumulate the values in a Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@400 PS10, Line 400: Substitute("table s > I think we want to use RunActionStdoutLines here. The tool should print the Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@407 PS10, Line 407: for (const auto& column : columns) { : // Check matched rows. : kvs.push_back(Substitute("$0 $1=$2", : column.first, column.second, column.second == "key" ? to_string(value) : ".*")); : } : string line_pattern(R"*(\()*"); : line_pattern += JoinStrings(kvs, ", "); : line_pattern += (")"); : ASSERT_STRINGS_ANY_MATCH(lines, line_pattern); : } : // Check total count. > It'd be possible to simplify this a bit with JoinStrings-- you could get ri Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@421 PS10, Line 421: > Remove. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2361 PS10, Line 2361: : // Create the src table and write some data to it. > Right now you are not using the client. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2365 PS10, Line 2365: e > nit: Use a value instead of reference. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2383 PS10, Line 2383: ableName, &table)); > C++ raw string literals would help make this more readable. See https://en. thanks for introducing it to me! http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2404 PS10, Line 2404: ,$0]])*", mid), > You can use the 'client' you created above to insert one more row that has Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2414 PS10, Line 2414: total_rows, total_rows); : RunScanTableCheck(kTableName, > Not used. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2418 PS10, Line 2418: > nit: Value instead of reference. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2420 PS10, Line 2420: } : : TEST_F(ToolTest, TestScanTableProjection) { : NO_FATALS(StartExternalMiniCluster()); : string master_addr = cluster_->master()->bound_rpc_addr().ToString(); : : const string kTableName = "kudu.table.scan.projection"; : : // Create the src table and write some da > I think using TestWorkload's default schema is fine, since this schema is j Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2454 PS10, Line 2454: NO_FATALS(StartExternalMiniCluster()); : string master_addr = cluster_->master()->bound_rpc_a > Unused. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2460 PS10, Line 2460: TestWorkload ww(cluster_.get()); : ww.set_table_name(kTableName); : ww.set_num_replicas(1); : ww.set_write_pattern(TestWorkload::INSERT_SEQUENTIAL_ROWS); : ww.set_num_write_threads(1); : ww.Setup(); : ww.Start(); : ASSERT_EVENTUALLY([&]() { : ASSERT_GE(ww.rows_inserted(), 1000); > Again, should be able to use TestWorkload's default schema for this. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2498 PS10, Line 2498: > Don't need this once the test works- the asserts should log the useful bit. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@91 PS10, Line 91: Builder builder; : ReplicaController::SetVisibility(&builder, ReplicaController::Visibility::ALL); : client::sp::shared_ptr<KuduClient> client; : RETURN_NOT_OK(builder > Update this in the same way as I suggested updating the commit message. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@95 PS10, Line 95: .master_server_addrs(master_addresses) : .Build(&client)); > Why don't we want to show the values by default? The most use scenario is to compare row count and time cost for each tablets to find out performance problem for bad designed schema, data skew, or some other reasons, so if we show the values by default, the terminal will be full of rows which are not we really care about. The secondary use scenario is to check the value. http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@186 PS10, Line 186: > If you need state, put it in a class. Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@200 PS10, Line 200: RETURN_NOT > Please leave this as client::sp::shared_ptr. It's intentional that the pref Done http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@445 PS10, Line 445: "as a JSON array" }) > I think this is enough extra code that it should go in its own file. Maybe Done -- To view, visit http://gerrit.cloudera.org:8080/12167 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8 Gerrit-Change-Number: 12167 Gerrit-PatchSet: 11 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Sat, 26 Jan 2019 17:06:25 +0000 Gerrit-HasComments: Yes
