Dinesh Bhat has posted comments on this change.

Change subject: [tools] added insert-generated-rows into kudu tools

Patch Set 12:


File src/kudu/tools/tool_action_test.cc:

PS12, Line 21: options
s/options/option ?

Line 33: //     --num_threads=1 \
don't you need master_addresses here as required params ? also applicable in 
examples below i guess.

PS12, Line 75: master server(s) location
as per the code below there is no default_params for master server location 
right ? so this comment may be invalid.

Line 80: //     bench_c_00_03
Same as above. Also here and above, we could keep this name intuitive to 
reflect this as a table_name, say table_bench_xx or something.

Line 81: //
I liked the documentation idea of these options, good stuff.

Line 119: using kudu::Schema;
I think Schema is not being used anywhere.

Line 131: using kudu::client::sp::shared_ptr;
Actually a basic Qn here: why do we use this special shared_ptr  as opposed to 
std::shared_ptr ?

PS12, Line 142: 1000000
Nit: Not sure if you want to insert 1 million by default, if I am a curious 
user of this tool, I would like to just see few rows inserted unless you want 
to exercise the buffer limit also by pushing this number higher. If I am a 
serious user, I know what flag to set. My goal would be to reduce the amount of 
time taken by this test for a novice/curious user.

PS12, Line 147: inserter
nit: rename it to loadgen threads or load generator threads here and above ?

Line 194:   int64_t NextImpl() {
Do we want to use uint64_t here ? Although Random.Next64() seems to be filling 
in only lower 62 bits at the moment, it is better to take a safer route here.

PS12, Line 215: != 0
this check is prolly redundant ?

PS12, Line 285: case UNKNOWN_DATA:  // fall-through
redundant ?

Line 302:              FLAGS_buffer_flush_watermark_pct));
I vaguely recall cpp guideline saying next spill being after 4 spaces I guess, 
however ignore if I am wrong.

PS12, Line 314: FLAGS_num_rows_per_thread
consider using variable FLAGS_ directly to be consistent with other places.

PS12, Line 315: num_rows_per_thread == 0
given it's default value being 1M, what's this check for ?

Line 324:   if (row_count != nullptr) {
DCHECK or CHECK for this, unless we have a use case where it could actually be 
a nullptr ?

Line 403:   Stopwatch sw(Stopwatch::ALL_THREADS);
Stupid Qn: Does ALL_THREADS here mean all threads on CPU or all threads 
generated by this process ?  Also consider the alternative LOG_TIMING* here 
which hides all the Stopwatch details underneath, see rle.cc for an example 

PS12, Line 440: optional scan afterwards
I think this need not be made explicit since this is listed in optional param 
section anyways.

PS12, Line 452: kTableNameArg
Can we not make this table_name optional and create one if the user doesn't 
specify one ? That way this test can be self-sufficient. Otherwise this test 
requires the user to have create the table from another tool(or some other 

PS12, Line 455:   
Nit: Double space here.

Line 457:           "the inserted rows." })
Ideally, we would not want this behavior of test leaving the test data on the 
server to keep it consistent with rest of the tests(although this is not a 
GTEST per se). I strongly recommend using either a call to 'kudu table delete' 
using SubProcess::Call or may be client->DeleteTable(table_name) since we have 
all the details here to delete a table unless we specify another optional param 
of keep_table or something alike.

To view, visit http://gerrit.cloudera.org:8080/4412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to