You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/10/08 04:54:15 UTC
[kudu-CR] [tools] kudu tool loadgen: do not abort on error
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4672
Change subject: [tools] kudu tool loadgen: do not abort on error
......................................................................
[tools] kudu tool loadgen: do not abort on error
Do not abort loadgen tool if a generator thread encounters an error.
Instead, stop producing data and gracefully join.
Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
---
M src/kudu/tools/tool_action_test.cc
1 file changed, 103 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/4672/1
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [tools] introduced '--seq start' flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4672
to look at the new patch set (#3).
Change subject: [tools] introduced '--seq_start' flag
......................................................................
[tools] introduced '--seq_start' flag
Introduced '--seq_start' flag for 'kudu test loadgen' tool.
Use this new flag to specify initial value for generator in sequential
mode. The new flag facilitates running the tool against already
existing table multiple times.
Here is an example: an already existing table t3 has 5 columns.
The sequence of 3 runs below inserts 6000 rows in total
(3 runs * 1000 rows per thread * 2 threads)
with no duplicate values across all columns:
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=0
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=10000
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=20000
Also, introduced '--table_num_replicas' flag
to specify number of replicas for auto-created tables.
Renamed '--num_buckets' into '--table_num_buckets'.
Besides, do not abort loadgen tool if generator thread encounters
an error. Instead, stop generating data and gracefully join.
Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
---
M src/kudu/tools/tool_action_test.cc
1 file changed, 124 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/4672/3
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] introduced '--seq start' flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [tools] introduced '--seq_start' flag
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [tools] introduced '--seq start' flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4672
to look at the new patch set (#2).
Change subject: [tools] introduced '--seq_start' flag
......................................................................
[tools] introduced '--seq_start' flag
Introduced '--seq_start' flag for 'kudu test loadgen' tool.
Use this new flag to specify initial value for generator in sequential
mode. The new flag facilitates running the tool against already
existing table multiple times.
Here is an example: an already existing table t3 has 5 columns.
The sequence of 3 runs below inserts 6000 rows in total
(3 runs * 1000 rows per thread * 2 threads)
with no duplicate values across all columns:
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=0
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=10000
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=20000
Also, introduced '--table_num_replicas' flag
to specify number of replicas for auto-created tables.
Renamed '--num_buckets' into '--table_num_buckets'.
Besides, do not abort loadgen tool if generator thread encounters
an error. Instead, stop generating data and gracefully join.
Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
---
M src/kudu/tools/tool_action_test.cc
1 file changed, 124 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/4672/2
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [tools] kudu tool loadgen: do not abort on error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools] kudu tool loadgen: do not abort on error
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4672/1/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:
PS1, Line 340: if (!session) {
: *status = Status::RuntimeError("cannot create a new session");
: return;
: }
How is this case possible?
PS1, Line 347: #ifdef BREAK_NOT_OK
: #undef BREAK_NOT_OK
: #endif
This isn't exactly safe if BREAK_NOT_OK is actually something important, though. Maybe just:
#define BREAK_NOT_OK
...
<use BREAK_NOT_OK>
#undef BREAK_NOT_OK
And if BREAK_NOT_OK is actually #defined, rename it to something more obscure?
Alternatively, how about encapsulating all of this logic in a helper method which returns a Status, then you don't need the unorthodox control flow that BREAK_NOT_OK provides, right?
Line 393: if (FLAGS_show_first_n_errors > 0) {
Is this check actually needed? Seems like if it were 0, the loop condition would be false and the loop would be skipped.
Line 396: unique_lock<mutex> lock(cerr_lock);
Nit: use lock_guard; unique_lock is only needed if you expect to manually unlock() or if you need some additional customization.
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [tools] introduced '--seq start' flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [tools] introduced '--seq_start' flag
......................................................................
[tools] introduced '--seq_start' flag
Introduced '--seq_start' flag for 'kudu test loadgen' tool.
Use this new flag to specify initial value for generator in sequential
mode. The new flag facilitates running the tool against already
existing table multiple times.
Here is an example: an already existing table t3 has 5 columns.
The sequence of 3 runs below inserts 6000 rows in total
(3 runs * 1000 rows per thread * 2 threads)
with no duplicate values across all columns:
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=0
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=10000
kudu test loadgen 127.0.0.1 --table_name=t3 --num_threads=2 \
--num_rows_per_thread=1000 --seq_start=20000
Also, introduced '--table_num_replicas' flag
to specify number of replicas for auto-created tables.
Renamed '--num_buckets' into '--table_num_buckets'.
Besides, do not abort loadgen tool if generator thread encounters
an error. Instead, stop generating data and gracefully join.
Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Reviewed-on: http://gerrit.cloudera.org:8080/4672
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tools/tool_action_test.cc
1 file changed, 124 insertions(+), 76 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] kudu tool loadgen: do not abort on error
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [tools] kudu tool loadgen: do not abort on error
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4672/1/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:
PS1, Line 340: if (!session) {
: *status = Status::RuntimeError("cannot create a new session");
: return;
: }
> How is this case possible?
I just looked at client::NewSession() as an black box. Yes, in current implementation there would be std::bad_alloc exception.
OK, I'll remove this.
PS1, Line 347: #ifdef BREAK_NOT_OK
: #undef BREAK_NOT_OK
: #endif
> This isn't exactly safe if BREAK_NOT_OK is actually something important, th
That's a good idea. For some reason, not so good idea came to my mind when I was riding Caltrain and writing this :)
Line 393: if (FLAGS_show_first_n_errors > 0) {
> Is this check actually needed? Seems like if it were 0, the loop condition
I agree -- here it's not needed; that's just a remnant from multiple iterations here.
However, I re-worked this back to output errors from one generator at once and now it's needed.
Line 396: unique_lock<mutex> lock(cerr_lock);
> Nit: use lock_guard; unique_lock is only needed if you expect to manually u
Done
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [tools] introduced '--seq start' flag
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools] introduced '--seq_start' flag
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0899168b31d4e3a2d53fff7db669a0840452f557
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No