You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/12/13 07:43:50 UTC
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Hello David Ribeiro Alves, Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5488
to review the following change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
This adds a test for the case where an ALTER TABLE adds a column, and
then we read the old history of that row from prior to the addition of
the column. We make sure that the default value of the new column is
returned.
Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 47 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/5488/1
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
Patch Set 3: Code-Review+2
Carrying David's +2
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5488
to look at the new patch set (#2).
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
This adds a test for the case where an ALTER TABLE adds a column, and
then we read the old history of that row from prior to the addition of
the column. We make sure that the default value of the new column is
returned.
Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 47 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/5488/2
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5488/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
PS1, Line 891: this seems odd that the
: // "latest observed" timestamp is one higher than the thing we wrote
> the latest observed timestamp is read from the clock just before the scanne
you mean from the write response, right?
Is that a bug or a feature? i.e I would sort of expect that the "latest observed" would be exactly my timestamp, because my write didn't observe the timestamp after it. For example, if the timestamp after it was used to UPSERT on top of my INSERT, then my INSERT didn't "observe" the UPSERT (it would have failed had it observed it)
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
This adds a test for the case where an ALTER TABLE adds a column, and
then we read the old history of that row from prior to the addition of
the column. We make sure that the default value of the new column is
returned.
Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Reviewed-on: http://gerrit.cloudera.org:8080/5488
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 47 insertions(+), 1 deletion(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5488/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
PS1, Line 891: this seems odd that the
: // "latest observed" timestamp is one higher than the thing we wrote
> sorry, I think I described the problem backwards.
I traced through the code a bit more and decided this is an off-by-one bug in MvccManager. Filed KUDU-1813
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5488/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
PS1, Line 891: this seems odd that the
: // "latest observed" timestamp is one higher than the thing we wrote
> you mean from the write response, right?
sorry, I think I described the problem backwards.
I think the client _is_ getting the exact timestamp of its write as the "latest observed timestamp". However, a READ_AT_SNAPSHOT with the SetSnapshotRaw equal to that time is not seeing the row inserted at that time (ts1 is the time of the latest insert). So maybe when we are creating the snapshot in the scan code, we are off-by-one somewhere in determining what's committed?
I see that consistency-itest works around the same "bug" by adding a + 1 to the timestamps provided to the GetRowCount() helper. Should I file a JIRA about this?
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5488/1/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
PS1, Line 891: this seems odd that the
: // "latest observed" timestamp is one higher than the thing we wrote
the latest observed timestamp is read from the clock just before the scanner response.
--
To view, visit http://gerrit.cloudera.org:8080/5488
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes