You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/12/07 04:02:06 UTC
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Hello Jean-Daniel Cryans, Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5395
to review the following change.
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
KUDU-1792 It is possible to overwrite range partitions
This fixes another bug (see KUDU-1750) in the master code which detects
overlaps in range partitions during alter table operations. A new test
is included which attempts to exhaustively cover all cases of conflict
between pairs of ranges being added. It also adds a lot of coverage for
drop conflicts between pairs of ranges, although the coverage is not
intended to be exhaustive.
The next step if we want to test this space even more would be to
increase the number of ranges involved in conflicts to three or more,
but since this amplifies the search space so much, it is probably best
done as part of a randomized fuzz test.
Change-Id: I89ae162f33de20169848091e518315853be30460
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 321 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/5395/1
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5395/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
PS1, Line 1533: CHECK
ASSERT
Line 1540: CHECK_OK(row->SetInt32("c0", *value));
Can you ASSERT_OK here too?
Line 1743: /// Unbounded Above / Unbounded Above
This comment is misplaced, right? Isn't the comment on L1740-L1741 supposed to apply here?
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
Patch Set 2: Code-Review+2
Looks good though we do need to fix those Python tests.
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5395
to look at the new patch set (#3).
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
KUDU-1792 It is possible to overwrite range partitions
This fixes another bug (see KUDU-1750) in the master code which detects
overlaps in range partitions during alter table operations. A new test
is included which attempts to exhaustively cover all cases of conflict
between pairs of ranges being added. It also adds a lot of coverage for
drop conflicts between pairs of ranges, although the coverage is not
intended to be exhaustive.
The next step if we want to test this space even more would be to
increase the number of ranges involved in conflicts to three or more,
but since this amplifies the search space so much, it is probably best
done as part of a randomized fuzz test.
Change-Id: I89ae162f33de20169848091e518315853be30460
---
M python/kudu/tests/test_client.py
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
3 files changed, 329 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/5395/3
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
Patch Set 1:
(4 comments)
It looks like the python failures might be legitimate, in which case there are python tests relying on the buggy behavior.
http://gerrit.cloudera.org:8080/#/c/5395/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
PS1, Line 1533: CHECK
> ASSERT
Done
Line 1540: CHECK_OK(row->SetInt32("c0", *value));
> Can you ASSERT_OK here too?
No, because it's in a lamda with a non-void return type.
Line 1597: } else if (!lower_bound) {
> warning: don't use else after return [readability-else-after-return]
Done
Line 1743: /// Unbounded Above / Unbounded Above
> This comment is misplaced, right? Isn't the comment on L1740-L1741 supposed
Done
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5395
to look at the new patch set (#2).
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
KUDU-1792 It is possible to overwrite range partitions
This fixes another bug (see KUDU-1750) in the master code which detects
overlaps in range partitions during alter table operations. A new test
is included which attempts to exhaustively cover all cases of conflict
between pairs of ranges being added. It also adds a lot of coverage for
drop conflicts between pairs of ranges, although the coverage is not
intended to be exhaustive.
The next step if we want to test this space even more would be to
increase the number of ranges involved in conflicts to three or more,
but since this amplifies the search space so much, it is probably best
done as part of a randomized fuzz test.
Change-Id: I89ae162f33de20169848091e518315853be30460
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 323 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/5395/2
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1792 It is possible to overwrite range partitions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: KUDU-1792 It is possible to overwrite range partitions
......................................................................
KUDU-1792 It is possible to overwrite range partitions
This fixes another bug (see KUDU-1750) in the master code which detects
overlaps in range partitions during alter table operations. A new test
is included which attempts to exhaustively cover all cases of conflict
between pairs of ranges being added. It also adds a lot of coverage for
drop conflicts between pairs of ranges, although the coverage is not
intended to be exhaustive.
The next step if we want to test this space even more would be to
increase the number of ranges involved in conflicts to three or more,
but since this amplifies the search space so much, it is probably best
done as part of a randomized fuzz test.
Change-Id: I89ae162f33de20169848091e518315853be30460
Reviewed-on: http://gerrit.cloudera.org:8080/5395
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M python/kudu/tests/test_client.py
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
3 files changed, 329 insertions(+), 4 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5395
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I89ae162f33de20169848091e518315853be30460
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>