You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2019/10/29 23:30:20 UTC

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation 

Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14578


Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test in a loop over 100 times.

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 52 insertions(+), 23 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test successfully in a loop over 1000 times using dist-test

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Reviewed-on: http://gerrit.cloudera.org:8080/14578
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 64 insertions(+), 29 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

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

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

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test successfully in a loop over 1000 times using dist-test

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 64 insertions(+), 29 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Thu, 31 Oct 2019 05:09:38 +0000
Gerrit-HasComments: No

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 4:

(3 comments)

Whoops, it seems I forgot to publish my review yesterday when I took a look at it.

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@86
PS3, Line 86: GetNumCreateTabletRPCs
I think it's necessary to wrap every call to this function with NO_FATALS(), otherwise if GetInt64Metrics() fails, the failure would be found only later on, and non-valid results would be used.

An alternative approach would be making this function return Status and keep ASSERT_OK() at call sites.


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@144
PS3, Line 144:   bool in_progress = true;
> Nice.
Indeed.


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@145
PS3, Line 145:   while (in_progress) {
             :     LOG(INFO) << "Waiting for the master to successfully create the table...";
             :     SleepFor(MonoDelta::FromMilliseconds(100));
             :     ASSERT_OK(client_->IsCreateTableInProgress(kTableName, &in_progress));
             :   }
nit: this might be replaced with ASSERT_EVENTUALLY:

ASSERT_EVENTUALLY([&]{
  bool in_progress = true;
  ASSERT_OK(client_->IsCreateTableInProgress(kTableName, &in_progress));
  ASSERT_FALSE(in_progress);
});



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Wed, 30 Oct 2019 21:13:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@144
PS3, Line 144:   bool in_progress = true;
Nice.


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@151
PS3, Line 151:   // At this point, table has been successfully created along with the tablets across the replicas.
Nit: "along with the tablets across the replicas" seems like a misnomer: what does it mean for a table to be created "across" replicas?


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@152
PS3, Line 152:   // All the replica tablet servers should be left with only one tablet, eventually,
Nit: "replica tablet servers" also reads like a misnomer.


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@151
PS3, Line 151:   // At this point, table has been successfully created along with the tablets across the replicas.
             :   // All the replica tablet servers should be left with only one tablet, eventually,
             :   // since the tablets which failed to get created properly should get deleted.
             :   // It's possible there are some delete tablet RPCs still inflight and not processed yet.
As per contributing.adoc:

  The Kudu team allows line lengths of 100 characters per line, rather than Google's standard of 80. Try to
  keep under 80 where possible, but you can spill over to 100 or so if necessary.


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@174
PS3, Line 174:   std::array<int64_t, kNumReplicas> num_create_attempts_arr{};
Could do the same thing with:

  vector<int64_t> num_create_attempts(kNumReplicas);

std::array isn't wrong per se, it's just pretty unusual.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Oct 2019 00:37:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

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

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

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test in a loop over 100 times.

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 57 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

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

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

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test in a loop over 100 times.

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 57 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

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

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

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test successfully in a loop over 1000 times using dist-test

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 65 insertions(+), 29 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

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

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

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................

[master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

Commit d119d529 deflaked the test by wrapping the verification
of tablets under ASSERT_EVENTUALLY. However crux of the problem
was that test wasn't waiting for the table creation to complete
which explains the flakiness observed.

Despite fixing the bug, ASSERT_EVENTUALLY is still needed
because there is a theoretical case of Delete Tablet RPCs
still not being processed at the time of verification of
the tablets.

Instead of simply checking one tablet server, with this
change checks are made across all the tablet servers.

This change also adds verification such that once
table creation is complete no further Create Tablet RPCs
are issued/serviced.

Tests:
- Ran the test in a loop over 100 times.

Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
---
M src/kudu/integration-tests/create-table-itest.cc
1 file changed, 59 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14578/5/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14578/5/src/kudu/integration-tests/create-table-itest.cc@145
PS5, Line 145: LOG(INFO) << "Waiting for the master to successfully create the table...";
nit: this might be dropped -- our tests are too chatty even without INFO logs from the test code itself



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Wed, 30 Oct 2019 22:55:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14578/5/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14578/5/src/kudu/integration-tests/create-table-itest.cc@145
PS5, Line 145: bool in_progress = true;
> nit: this might be dropped -- our tests are too chatty even without INFO lo
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Wed, 30 Oct 2019 23:10:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 5:

(4 comments)

> Patch Set 4: Code-Review+2
> 
> (1 comment)

http://gerrit.cloudera.org:8080/#/c/14578/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14578/4//COMMIT_MSG@27
PS4, Line 27: - Ran the test successfully in a loop over 1000 times using dist-test
> BTW, you should use dist-test for this; you can loop a test 1000 times in j
Done.


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@86
PS3, Line 86: urn itest::GetInt64Met
> I think it's necessary to wrap every call to this function with NO_FATALS()
Done


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@144
PS3, Line 144:   ASSERT_EVENTUALLY([&] {
> Indeed.
Ack


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@145
PS3, Line 145:     LOG(INFO) << "Waiting for the master to successfully create the table...";
             :     bool in_progress = true;
             :     ASSERT_OK(client_->IsCreateTableInProgress(kTableName, &in_progress));
             :     ASSERT_FALSE(in_progress);
             :   }
> nit: this might be replaced with ASSERT_EVENTUALLY:
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Wed, 30 Oct 2019 22:43:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 3:

(5 comments)

> Patch Set 3:
> 
> Build Started http://jenkins.kudu.apache.org/job/kudu-gerrit/19350/

 Error Message
 /home/jenkins-slave/workspace/kudu-master/3/src/kudu/integration-tests/create- table-itest.cc:171
       Expected: kNumReplicas
       Which is: 3
 To be equal to: num_replicas_found
       Which is: 2

Test failed because table is considered created once majority of replica tablets have been successfully created. So it's not necessary for all the replica tablets to be created.

I'll update the test.

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc
File src/kudu/integration-tests/create-table-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@144
PS3, Line 144:   bool in_progress = true;
> Nice.
Ack


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@151
PS3, Line 151:   // At this point, table has been successfully created along with the tablets across the replicas.
> Nit: "along with the tablets across the replicas" seems like a misnomer: wh
Done


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@152
PS3, Line 152:   // All the replica tablet servers should be left with only one tablet, eventually,
> Nit: "replica tablet servers" also reads like a misnomer.
Done


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@151
PS3, Line 151:   // At this point, table has been successfully created along with the tablets across the replicas.
             :   // All the replica tablet servers should be left with only one tablet, eventually,
             :   // since the tablets which failed to get created properly should get deleted.
             :   // It's possible there are some delete tablet RPCs still inflight and not processed yet.
> As per contributing.adoc:
Done


http://gerrit.cloudera.org:8080/#/c/14578/3/src/kudu/integration-tests/create-table-itest.cc@174
PS3, Line 174:   std::array<int64_t, kNumReplicas> num_create_attempts_arr{};
> Could do the same thing with:
In this case, the size is fixed and known at compile time so std::array is more appropriate.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Oct 2019 18:40:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation

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

Change subject: [master] KUDU-2963 Fix and enhance TestCreateWhenMajorityOfReplicasFailCreation
......................................................................


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14578/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14578/4//COMMIT_MSG@27
PS4, Line 27: - Ran the test in a loop over 100 times.
BTW, you should use dist-test for this; you can loop a test 1000 times in just a few minutes.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9943200dbf330481996e58e27744fe9fb49267f
Gerrit-Change-Number: 14578
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Oct 2019 21:11:05 +0000
Gerrit-HasComments: Yes