Alexey Serbin has posted comments on this change.

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

Patch Set 12:

File src/kudu/tools/

PS11, Line 358:   }
              :   if (total_row_count != nullptr) {
              :     *total_row_count = accumulate(row_count.begin(), 
row_count.end(), 0UL);
              :   }
> Looks like it's still here?
Oops, my bad.  I'm surprised myself -- probably, I got distracted.  Let me 
remove it, though.
File src/kudu/tools/

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

Line 33: //     --num_threads=1 \
> don't you need master_addresses here as required params ? also applicable i
Good catch -- these examples became stale once I switched to the mandatory 
master(s) location.  Will update

PS12, Line 75: master server(s) location
> as per the code below there is no default_params for master server location
Yes, correct -- since changing to use master addresses as mandatory parameter, 
this no longer has default value.  Will update.

Line 80: //     bench_c_00_03
> Same as above. Also here and above, we could keep this name intuitive to re

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
It's just a typedef: it might be either std::shared_ptr or 
std::tr1::shared_ptr, check $REPO_ROOT/src/kudu/client/shared_ptr.h  That's 
because Kudu C++ client API uses this notation to be compatible with C++03.

PS12, Line 142: 1000000
> Nit: Not sure if you want to insert 1 million by default, if I am a curious
1M rows are inserted really fast: at my laptop it's just 5 seconds.  Also, the 
idea was to create some _load_ :)  With 1K rows there is no load at all.  
Besides, take into the consideration that the original insert-generated-rows 
tools was running with no limit at generated rows at all.

So, what do you think would be the right number here?

Line 194:   int64_t NextImpl() {
> Do we want to use uint64_t here ? Although Random.Next64() seems to be fill
OK, that makes sense.  Will update to uint64_t.

PS12, Line 215: != 0
> this check is prolly redundant ?
As I remember, Tidy Bot thinks it's better for readability: that's the reason I 
left it here.  I'll try to get rid of it -- let's see what it will say.

PS12, Line 285: case UNKNOWN_DATA:  // fall-through
> redundant ?
Yes, it seems so.  I copied the switch enumerators from some place and left 
this UNKNOWN_DATA as is.  Will remove.

Line 302:              FLAGS_buffer_flush_watermark_pct));
> I vaguely recall cpp guideline saying next spill being after 4 spaces I gue
QtCreator's editor does this sometimes.  Will fix.

PS12, Line 314: FLAGS_num_rows_per_thread
> consider using variable FLAGS_ directly to be consistent with other places.
I wanted to have a local variable since I access it in multiple places.

PS12, Line 315: num_rows_per_thread == 0
> given it's default value being 1M, what's this check for ?
Take a look at the comment for the '--num_rows_per_thread': 0 means no limit.

Line 324:   if (row_count != nullptr) {
> DCHECK or CHECK for this, unless we have a use case where it could actually
I don't see why DCHECK or CHECK is needed here.  One can pass nullptr as 
rowcount: that means there isn't any interest in getting the count.  It's a 
ubiquitous notation for pointer out parameters, which I want to have here, even 
if nobody uses it right now.

Line 403:   Stopwatch sw(Stopwatch::ALL_THREADS);
> Stupid Qn: Does ALL_THREADS here mean all threads on CPU or all threads gen
Threads in this process, so the monitoring/main thread will also be counted in.

I don't want LOG_TIMING, since I want to output that information in my own 
format, not what LOG_TIMING provides.

PS12, Line 440: optional scan afterwards
> I think this need not be made explicit since this is listed in optional par
I want this to be present to emphasize that there are two parts: the write and 
the read one, even if the latter is optional.  That's an essence of this test 
in a terse phrasing, otherwise it would be necessary to read through all the 
list of options to understand about the read part.

PS12, Line 452: kTableNameArg
> Can we not make this table_name optional and create one if the user doesn't
The original approach was to be able to use any table structure, not only some 
fixed pre-generated one which the test would create.  So, this test retrieves 
the table schema out of existing table it's pointed at.  That's because 
specifying custom table structure via parameters of this test would be 
We can add this an option, with pairing auto-delete option. In case of 
auto-created table we can safely delete it if nobody asks to leave it alone 
after the test is completed.

PS12, Line 455:   
> Nit: Double space here.
Yes, here and elsewhere in the description for parameters: this is intentional 
(that's easier for reading, IMO).  Or we have some sort of style guide for 
having one space for those?

Line 457:           "the inserted rows." })
> Ideally, we would not want this behavior of test leaving the test data on t
I'm strongly opposed to auto-deleting any data by the test in its current 
implementation, since this test might be run in production environment.  
Nothing prevents a user pointing to already existing table with some data, and 
I don't want to delete any useful data along with the data generated by the 

To me, using the approach you are advocating makes more sense in case of 
auto-created tables, because in that case we can have a guarantee the table was 
created by the test and not existed before.  I think we can use auto-generated 
table names for that and be safe.  I'll take a look at that approach.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I332927c0b928c9c4fb81a8e26f5c9ed7565299ad
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to