[kudu-CR] Add a benchmark test for tablet deletion

2017-11-01 Thread Adar Dembo (Code Review) 
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Reviewed-on: http://gerrit.cloudera.org:8080/8420
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 53 insertions(+), 0 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:39:27 +
Gerrit-HasComments: No

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Hao Hao (Code Review) 
Hello Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8420

to look at the new patch set (#3).

Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 53 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/8420/3
--
To view, visit http://gerrit.cloudera.org:8080/8420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Hao Hao (Code Review) 
Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2293
PS2, Line 2293:   uint64_t rows = FLAGS_delete_tablet_bench_num_flushes;
> Not needed anymore?
Done


http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2327
PS2, Line 2327: for
> Nit: remove this word
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:34:33 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2293
PS2, Line 2293:   uint64_t rows = FLAGS_delete_tablet_bench_num_flushes;
Not needed anymore?


http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2327
PS2, Line 2327: for
Nit: remove this word



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:22:50 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Hao Hao (Code Review) 
Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131
PS1, Line 131: DEFINE_int32(delete_tablet_bench_num_flushes, 200,
 :  "Number of disk row sets to flush in the delete 
tablet benchmark");
 :
> Nit: the name and description of this new flag suggests that it's part of t
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293
PS1, Line 2293:   uint64_t rows = FLAGS_delete_tablet_bench_num_flushes;
> I don't think this is necessary; the other benchmark in this test isn't con
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295
PS1, Line 2295:   // Collect some related metrics.
  :   scoped_refptr block_count =
> How long does this take to run in slow mode? If it's less than 5s, I don't
With 'delete_tablet_bench_num_flushes' default to 200, the test ran for 3s. So 
I will remove it.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299
PS1, Line 2299:   scoped_refptr container =
> Nit: terminate with a period.
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300
PS1, Line 2300:   METRIC_log_block_manager_containers.Instantiate(
> Isn't this guaranteed to be true by the test harness itself? Perhaps it can
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303
PS1, Line 2303:   METRIC_log_block_manager_holes_punched.Instantiate(
  :   mini_server_->server()->metric_entity());
  :
  :   // Put some data in the tablet. We insert rows and flush 
immediately to
  :   // ensure that there is enough blocks on disk to run the 
benchmark.
  :   for (int i = 0; i < rows; i++) {
  : NO_FATALS(InsertTestRowsRemote(i, 1));
  : ASSERT_OK(tablet_replica_->tablet()->Flush());
  :   }
> I presume this is okay to execute even when block_manager=file?
Right, it is safe to do so.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314
PS1, Line 2314: ablet from within
> Nit: replace with "run the"
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316
PS1, Line 2316:   // by the test code.
> Nit: new code should use the shorter NO_FATALS() macro.
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335
PS1, Line 2335:   // Log the related metrics.
> No latency is being measured here.
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344
PS1, Line 2344:   // Verify that the tablet exists
> You can log this regardless of block manager, no?
Right, will remove the condition.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:18:35 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Hao Hao (Code Review) 
Hello Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8420

to look at the new patch set (#2).

Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 55 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/8420/2
--
To view, visit http://gerrit.cloudera.org:8080/8420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131
PS1, Line 131: DEFINE_int32(single_threaded_delete_tablet_bench_insert_rows, 
500,
 :  "Number of rows to insert in the testing phase of 
the single threaded "
 :  "tablet server delete tablet benchmark");
Nit: the name and description of this new flag suggests that it's part of the 
existing "insert latency micro-benchmark". Perhaps call it 
"delete_tablet_bench_num_flushes" and describe it with "Number of disk row sets 
to flush in the delete tablet benchmark"?


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293
PS1, Line 2293: #ifdef NDEBUG
I don't think this is necessary; the other benchmark in this test isn't 
conditioned on RELEASE mode.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295
PS1, Line 2295:   uint64_t rows = AllowSlowTests() ?
  :   
FLAGS_single_threaded_delete_tablet_bench_insert_rows : 10;
How long does this take to run in slow mode? If it's less than 5s, I don't 
think it's worth the condition.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299
PS1, Line 2299:   // Verify that the tablet exists
Nit: terminate with a period.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300
PS1, Line 2300:   
ASSERT_TRUE(mini_server_->server()->tablet_manager()->LookupTablet(kTabletId, 
));
Isn't this guaranteed to be true by the test harness itself? Perhaps it can be 
omitted in favor of operating on tablet_replica_ directly?


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303
PS1, Line 2303:   scoped_refptr block_count =
  :   
METRIC_log_block_manager_blocks_under_management.Instantiate(
  :   mini_server_->server()->metric_entity(), 0);
  :   scoped_refptr container =
  :   METRIC_log_block_manager_containers.Instantiate(
  :   mini_server_->server()->metric_entity(), 0);
  :   scoped_refptr holes_punched =
  :   METRIC_log_block_manager_holes_punched.Instantiate(
  :   mini_server_->server()->metric_entity());
I presume this is okay to execute even when block_manager=file?


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314
PS1, Line 2314: do tablet deletion
Nit: replace with "run the"


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316
PS1, Line 2316: ASSERT_NO_FATAL_FAILURE(InsertTestRowsRemote(i, 1));
Nit: new code should use the shorter NO_FATALS() macro.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335
PS1, Line 2335:   // Send the call and measure the latency.
No latency is being measured here.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344
PS1, Line 2344:   if (FLAGS_block_manager == "log") {
You can log this regardless of block manager, no?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:37:00 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

2017-10-30 Thread Hao Hao (Code Review) 
Hao Hao has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8420


Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 66 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/8420/1
--
To view, visit http://gerrit.cloudera.org:8080/8420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao