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 2017/02/06 22:59:25 UTC
[kudu-CR] compaction: Add additional validation in DeltaTracker
Hello David Ribeiro Alves, Jean-Daniel Cryans,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5919
to review the following change.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
compaction: Add additional validation in DeltaTracker
This adds DEBUG-mode validation to AtomicUpdateStores() in the
DeltaTracker. It also updates the doc comment on the method.
Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
3 files changed, 64 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5919/1
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
Patch Set 1:
(3 comments)
> Worth adding a test?
I didn't change any functionality, I simply added new DCHECKs. That's basically like adding a test that runs all the time in DEBUG mode.
http://gerrit.cloudera.org:8080/#/c/5919/1/src/kudu/tablet/delta_tracker.cc
File src/kudu/tablet/delta_tracker.cc:
PS1, Line 177: CHECK_OK
> weird that this is a check instead of dcheck when this is only executed in
It seems redundant to me but I can just make it a DCHECK.
PS1, Line 181: CHECK_LT
> add error messages. same below
Done
PS1, Line 184: CHECK_GT(first->delta_stats().max_timestamp(), second->delta_stats().max_timestamp());
> are we really sure that the timestamps don't overlap here? I ran into a buc
I see -- I think you are right. So in that case this should be DCHECK_GE.
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
compaction: Add additional validation in DeltaTracker
This adds DEBUG-mode validation to AtomicUpdateStores() in the
DeltaTracker. It also updates the doc comment on the method.
Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Reviewed-on: http://gerrit.cloudera.org:8080/5919
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile.h
M src/kudu/util/status.h
5 files changed, 87 insertions(+), 16 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Jean-Daniel Cryans: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5919
to look at the new patch set (#3).
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
compaction: Add additional validation in DeltaTracker
This adds DEBUG-mode validation to AtomicUpdateStores() in the
DeltaTracker. It also updates the doc comment on the method.
Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/deltafile.h
M src/kudu/util/status.h
5 files changed, 87 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5919/3
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5919
to look at the new patch set (#2).
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
compaction: Add additional validation in DeltaTracker
This adds DEBUG-mode validation to AtomicUpdateStores() in the
DeltaTracker. It also updates the doc comment on the method.
Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/util/status.h
4 files changed, 85 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5919/2
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
Patch Set 1:
Worth adding a test?
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] compaction: Add additional validation in DeltaTracker
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: compaction: Add additional validation in DeltaTracker
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5919/1/src/kudu/tablet/delta_tracker.cc
File src/kudu/tablet/delta_tracker.cc:
PS1, Line 177: CHECK_OK
weird that this is a check instead of dcheck when this is only executed in debug mode.
PS1, Line 181: CHECK_LT
add error messages. same below
PS1, Line 184: CHECK_GT(first->delta_stats().max_timestamp(), second->delta_stats().max_timestamp());
are we really sure that the timestamps don't overlap here? I ran into a buch of corner cases like this when adding reinserts with fuzz itest.
Say that we have a big batch of updates that touch a bunch of rows, then later on a compaction comes that ends up merging the rowsets and thus generating new deltas undo stores. if the batch is large enough there is the possibility that the updates will span more than one delta store i.e. that the max update of the two stores is the same
--
To view, visit http://gerrit.cloudera.org:8080/5919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I05fed515bc5d6a09484fadb61e73ac1346f6654a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes