You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "helifu (Code Review)" <ge...@cloudera.org> on 2018/10/31 07:19:18 UTC
[kudu-CR] KUDU-2529: Add a "-tables=" flag to the "kudu table list".
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/11360 )
Change subject: KUDU-2529: Add a "-tables=<tables>" flag to the "kudu table list".
......................................................................
Patch Set 9:
(17 comments)
sorry for the late reply, i thought the comments will submit automatically -_-#
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2165
PS5, Line 2165: tring&
> Nit: mind naming these 'ts_uuid' and 'ts_addr'? The "ts_" prefix is what we
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2170
PS5, Line 2170: workload.set_num_replicas(1);
> Nit: name this kNumTables
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2187
PS5, Line 2187: NO_FATALS(StartExternalMiniCluster());
> Is this actually necessary? Aren't you always guaranteed to have written "k
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2188
PS5, Line 2188: const string& kTableName = "table";
> Nit: with ASSERT_EQ, our convention is to list the "expected" value first,
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2194
PS5, Line 2194: ->Type(client::KuduColumnSchema::INT32)
> Nit: reformat as:
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2207
PS5, Line 2207:
> Nit: no need to separate these with a space.
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2210
PS5, Line 2210: string master_addr = cluster_->master()->bound_rpc_addr().ToString();
: string out;
> Nit: our convention is to separate operands from operators with a space. So
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2215
PS5, Line 2215: shared_ptr<KuduClient> client;
: ASSERT_OK(KuduClientBuilder()
> Could do a simpler foreach loop:
Done
http://gerrit.cloudera.org:8080/#/c/11360/5/src/kudu/tools/kudu-tool-test.cc@2230
PS5, Line 2230: // Replica's format.
: string ts_uuid = cluster_->tablet_server(0)->uuid();
: string ts_addr = cluster_->tablet_server(0)->bound_rpc_addr().ToString();
: string expect_replica = Substitute(" L $0 $1", ts_uuid, ts_addr);
:
: // Create some tables.
: const int kNumTables = 10;
: vector<string> table_names;
: for (int i = 0; i < kNumTables; ++i) {
: string table_name = Substitute("kudu.table_$0", i);
: table_names.push_back(table_name);
:
: TestWorkload workload(cluster_.get());
: workload.set_table_name(table_name);
: workload.set_num_replicas(1);
: workload.Setup();
: }
: std::sort(table_names.begin(), table_names.end());
:
: const auto& ProcessTables = [&] (const int num) {
: ASSERT_GE(num, 1);
: ASSERT_LE(num, kNumTables);
:
: vector<string> expected;
: expected.insert(expected.end(), table_names.begin(), table_names.begin() + num);
:
: string filter = "";
:
> This is almost identical to the "List all the tablets of all the tables" te
Done
http://gerrit.cloudera.org:8080/#/c/11360/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11360/6/src/kudu/tools/kudu-tool-test.cc@2170
PS6, Line 2170: workload.set_num_replicas(1);
> Got some trailing whitespace here and below; gerrit highlights this in red.
Done
http://gerrit.cloudera.org:8080/#/c/11360/6/src/kudu/tools/kudu-tool-test.cc@2171
PS6, Line 2171: workload.Setup();
:
: string out;
> I don't understand why this is the case; can you explain?
Done
http://gerrit.cloudera.org:8080/#/c/11360/6/src/kudu/tools/kudu-tool-test.cc@2186
PS6, Line 2186: TEST_F(ToolTest, TestRenameColumn) {
: NO_FATALS(StartExternalMiniCluster());
: const string& kTableName = "table";
: const string& kColumnName = "col.0";
: const string& kNewColumnName = "col_0";
:
: KuduSchemaBuilder schema_builder;
: sc
> Check out gutil/strings/join.h, there are functions that'll help you with t
Done
http://gerrit.cloudera.org:8080/#/c/11360/6/src/kudu/tools/kudu-tool-test.cc@2201
PS6, Line 2201: ASSERT_OK(schema_builder.Build(&schema));
> warning: 'push_back' is called inside a loop; consider pre-allocating the v
Done
http://gerrit.cloudera.org:8080/#/c/11360/6/src/kudu/tools/kudu-tool-test.cc@2205
PS6, Line 2205: workload.set_table_name(kTableName);
: workload.set_schema(schema);
: workload.set_num_replicas(1);
: workload.Setup();
:
: string master_addr = cluster_->master()->bound_rpc_addr().ToString();
: str
> You could share more of this between the two cases. Something like:
Done
http://gerrit.cloudera.org:8080/#/c/11360/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/11360/1/src/kudu/tools/tool_action_common.cc@105
PS1, Line 105: DEFINE_string(tables, "", "Tables to include (comma-separated list of table names)"
: "If not specified, includes all tables.");
:
> The new usage of FLAGS_tables doesn't "check" them; it just includes them i
Done
http://gerrit.cloudera.org:8080/#/c/11360/1/src/kudu/tools/tool_action_common.cc@457
PS1, Line 457: if (patterns.empty()) return true;
> Can you preserve the "// Consider no filter..." comment here?
Done
http://gerrit.cloudera.org:8080/#/c/11360/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11360/1/src/kudu/tools/tool_action_table.cc@194
PS1, Line 194: .AddRequiredParameter({ kMasterAddressesArg, kMasterAddressesArgDesc })
> This should just be "tables", and is likely the reason that all of the test
Done
--
To view, visit http://gerrit.cloudera.org:8080/11360
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I23006a3f3e4f157e6c6c10968b8692e89892d1ce
Gerrit-Change-Number: 11360
Gerrit-PatchSet: 9
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 31 Oct 2018 07:19:18 +0000
Gerrit-HasComments: Yes