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 <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
