You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2019/03/13 00:40:21 UTC
[kudu-CR] KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12735
to review the following change.
Change subject: KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
......................................................................
KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
This test had a couple of test-only bugs related to bookkeeping,
resulting in this test being flaky in certain scenarios. This patch
fixes those bugs:
1. Ensure that we don't generate duplicate keys in the same list (a.k.a.
RowSet) during the test because that can't occur "in the wild" and
the MergeIterator does not support it.
2. Ensure that we don't maintain more than one instance of a single
row key in the 'expected' result set. Switch to using an ordered map
to enforce that test invariant. An additional benefit to using that
data structure is that we can skip the sorting step when iterating
the expected results.
Before this change, the test had a failure rate of about 10% according
to the flaky test dashboard. After this change, the tests passes
reliably and succeeded 1024/1024 times when I looped it using dist-test:
http://dist-test.cloudera.org/job?job_id=mpercy.1552437266.46341
Change-Id: I5ca9cdfa4620b1a9d4144de2289cc80ac37060aa
---
M src/kudu/common/generic_iterators-test.cc
1 file changed, 59 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/12735/1
--
To view, visit http://gerrit.cloudera.org:8080/12735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5ca9cdfa4620b1a9d4144de2289cc80ac37060aa
Gerrit-Change-Number: 12735
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12735 )
Change subject: KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12735/1/src/kudu/common/generic_iterators-test.cc
File src/kudu/common/generic_iterators-test.cc:
http://gerrit.cloudera.org:8080/#/c/12735/1/src/kudu/common/generic_iterators-test.cc@258
PS1, Line 258: vector<List> all_ints;
> Hmm, I thought defining a one-arg List constructor means the compiler won't
You're allowed to create a container that only contains things not default-constructible. Since the container starts out empty and we only move things into it, it's not a problem.
--
To view, visit http://gerrit.cloudera.org:8080/12735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ca9cdfa4620b1a9d4144de2289cc80ac37060aa
Gerrit-Change-Number: 12735
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 13 Mar 2019 04:49:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12735 )
Change subject: KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12735/1/src/kudu/common/generic_iterators-test.cc
File src/kudu/common/generic_iterators-test.cc:
http://gerrit.cloudera.org:8080/#/c/12735/1/src/kudu/common/generic_iterators-test.cc@258
PS1, Line 258: vector<List> all_ints;
Hmm, I thought defining a one-arg List constructor means the compiler won't generate a default constructor, in which case how is it possible to create a vector<List>?
--
To view, visit http://gerrit.cloudera.org:8080/12735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ca9cdfa4620b1a9d4144de2289cc80ac37060aa
Gerrit-Change-Number: 12735
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Mar 2019 04:19:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12735 )
Change subject: KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
......................................................................
KUDU-2741: De-flake TestMergeIterator.TestDeDupGhostRows
This test had a couple of test-only bugs related to bookkeeping,
resulting in this test being flaky in certain scenarios. This patch
fixes those bugs:
1. Ensure that we don't generate duplicate keys in the same list (a.k.a.
RowSet) during the test because that can't occur "in the wild" and
the MergeIterator does not support it.
2. Ensure that we don't maintain more than one instance of a single
row key in the 'expected' result set. Switch to using an ordered map
to enforce that test invariant. An additional benefit to using that
data structure is that we can skip the sorting step when iterating
the expected results.
Before this change, the test had a failure rate of about 10% according
to the flaky test dashboard. After this change, the tests passes
reliably and succeeded 1024/1024 times when I looped it using dist-test:
http://dist-test.cloudera.org/job?job_id=mpercy.1552437266.46341
Change-Id: I5ca9cdfa4620b1a9d4144de2289cc80ac37060aa
Reviewed-on: http://gerrit.cloudera.org:8080/12735
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/common/generic_iterators-test.cc
1 file changed, 59 insertions(+), 40 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5ca9cdfa4620b1a9d4144de2289cc80ac37060aa
Gerrit-Change-Number: 12735
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>