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 2020/08/08 04:02:12 UTC

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions 

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16312


Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

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



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16312 )

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................


Patch Set 5: Verified+1

unrelated test failures in ClientTest.TestFailedDnsResolution and some java test


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 17 Aug 2020 14:50:18 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16312 )

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................


Patch Set 5: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 25 Aug 2020 16:09:07 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, 

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

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

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

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

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


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/16312/4
-- 
To view, visit http://gerrit.cloudera.org:8080/16312
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16312 )

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Reviewed-on: http://gerrit.cloudera.org:8080/16312
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 133 insertions(+), 0 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, 

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

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

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

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

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


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/16312/5
-- 
To view, visit http://gerrit.cloudera.org:8080/16312
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar, 

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

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

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

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

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


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16312 )

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 28 Aug 2020 06:02:06 +0000
Gerrit-HasComments: No

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, 

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

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

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

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

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


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/16312/6
-- 
To view, visit http://gerrit.cloudera.org:8080/16312
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, 

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

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

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

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................

[tserver] add test to reproduce KUDU-1587 conditions

Added a test to reproduce conditions described in KUDU-1587.
As of now, the test is disabled: it will be enabled once
KUDU-1587 is addressed.

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


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/16312/7
-- 
To view, visit http://gerrit.cloudera.org:8080/16312
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/16312
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16312 )

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4368
PS1, Line 4368: num_data_dirs
> Would it also make sense to reduce the number of apply threads to 1? Would 
Well, I'd rather keep it closer to "real" configurations.  That also helps to avoid extra changes introducing max pool size for the apply queue.  Or you think it's better to introduce control knobs for the apply queue max_size anyways (maybe, in a separate changelist)?


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4387
PS1, Line 4387: re the
> nit: incomplete?
Done


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4446
PS1, Line 4446:     EXPECT_LT(h->MaxValue(),
              :               kLatencyMs * 1000 * kNumCalls / base::NumCPUs());
              :     EXPECT_LT(h->ValueAtPercentile(99),
              :               kLatencyMs * 1000 * kNumCalls / (2 * base::NumCPUs()));
              :     EXPECT_LT(h->ValueAtPercentile(50),
              :               kLatencyMs * 1000 * kNumCalls / (4 * base::NumCPUs()));
> Making sure I understand these assertions: if we had a single core, we'd ex
Well, these are rather simple heuristics.  I'm not sure we want to draw some exact lines here based on the parameters for CoDel.

I added an extra comment to clarify on this.


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4463
PS1, Line 4463: 
              :     EXPECT_LT(h->MaxValue(), kNumCalls / 2);
              :     EXPECT_LT(h->MeanValue(), kNumCalls / 4);
> Same here, right? These multipliers will be adjusted depending on the imple
These are simple heuristics as well. They depend on the injected latency and the 'overloaded' threshold for the apply queue.

I added this as a comment.

BTW, the nice feature of the CoDel algorithm is that it has almost no configuration parameters.


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4468
PS1, Line 4468:   // Some calls should be rejected by the apply queue, so the maximum queue
              :   // time should be much lower than xxx.
> nit: move this above L4436? Or is this referring to the RPC queue?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 15 Aug 2020 00:15:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tserver] add test to reproduce KUDU-1587 conditions

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16312 )

Change subject: [tserver] add test to reproduce KUDU-1587 conditions
......................................................................


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4368
PS1, Line 4368: num_data_dirs
Would it also make sense to reduce the number of apply threads to 1? Would that give us any stronger guarantees? Or is the idea that it's better to test with multiple cores to be more realistic?


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4387
PS1, Line 4387: re the
nit: incomplete?


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4446
PS1, Line 4446:     EXPECT_LT(h->MaxValue(),
              :               kLatencyMs * 1000 * kNumCalls / base::NumCPUs());
              :     EXPECT_LT(h->ValueAtPercentile(99),
              :               kLatencyMs * 1000 * kNumCalls / (2 * base::NumCPUs()));
              :     EXPECT_LT(h->ValueAtPercentile(50),
              :               kLatencyMs * 1000 * kNumCalls / (4 * base::NumCPUs()));
Making sure I understand these assertions: if we had a single core, we'd expect the apply queue time to be far from `kLetancySec * kNumCalls` because some of the requests should have been rejected. And I suppose the exact value for the p50 and p99 calls will be determined based on the CoDel algorithm.


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4463
PS1, Line 4463: 
              :     EXPECT_LT(h->MaxValue(), kNumCalls / 2);
              :     EXPECT_LT(h->MeanValue(), kNumCalls / 4);
Same here, right? These multipliers will be adjusted depending on the implementation of CoDel, right? Or are these guarantees/measurements made by the algorithm?


http://gerrit.cloudera.org:8080/#/c/16312/1/src/kudu/tserver/tablet_server-test.cc@4468
PS1, Line 4468:   // Some calls should be rejected by the apply queue, so the maximum queue
              :   // time should be much lower than xxx.
nit: move this above L4436? Or is this referring to the RPC queue?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I515a1b26152680ee9b9361afcf84fec39b8f962d
Gerrit-Change-Number: 16312
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 Aug 2020 21:48:19 +0000
Gerrit-HasComments: Yes