David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files ......................................................................
Patch Set 13: (29 comments) http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/common/row_changelist.h File src/kudu/common/row_changelist.h: Line 349 > hmm, why remove these assertions? they've been quite useful in catching bug oh, I guess this is leftover from the version of the patch that didn't have ghosts in multiple DRSs, put them back. PS12, Line 309: > I guess this function is no longer meant to be update-specific, given you r Done http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: Line 528: "Undos: [@1(DELETE)] " > I think it's worth extending this test or adding a new one which does somet I added a new test that covers this stuff more thoroughly. http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 90: row_block_.reset(new RowBlock(iter_->schema(), num_in_block, &arena_)); > hm, was this a bug? I guess not really since we don't do flushing compactions (where we would need the MRS's arena in MergeCompactionInput::PrepareBlock()) but I figured it couldn't hurt, right? seems silly that we're putting the mutations in the arena but not the copy of the row. Line 255: // Compares two duplicate rows before compaction (and before the REDO->UNDO transformation). > At first before reading the code, I expected there was a precondition here yeah this happen before the transformation and serves the sole purpose of comparing two rows with the same key from two row sets to set 'previous_ghost' appropriately. One of the rows might have a log redo history. Made this more explicit. PS12, Line 256: -1 if 'left' is less recent than 'r > this phrasing is a bit odd. Is it equivalent to say: Returns -1 if 'left' i Done Line 261: DCHECK(right.redo_head != nullptr); > for the purposes of this DCHECK, seems like it's worth actually doing Advan Done PS12, Line 284: return ret; : } : > can you give the specific scenario here? iirc it's: Done PS12, Line 295: > its Done Line 299: // Update row 'a' @ 10 > nit: maybe more intuitive to write this as Done PS12, Line 453: :OK(); > this sounds like you're talking about a pre-existing state, but really you Done Line 589: return false; > is this recursion inherently depth-limited by the number of input rowsets? yeah, there can be at most n-1 ghosts per row where n is the number of rowsets in the compaction input Line 667: Arena* prepared_block_arena_; > doc Done Line 679: }; > doing this recursively seems dangerous, since it's bounded only by the numb it can, you're right, I was lazy. Done PS12, Line 698: > typo Done PS12, Line 708: Bi > typo: is Done Line 969: > nit: indentation seems off on this comment block. Done Line 988: > yea, this seems like a potentially serious issue. Mind filing a critical JI created KUDU-1760 Line 991: } > I know you didn't write this but is it right not to use an arena here? I guess the point here is that there is no advantage in copying the row to the arena since dst_row comes from a row_block on the stack and it's lifetime is already controlled. http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: PS12, Line 167: row is found in mu > or "when a row appears in multiple RowSets" Done PS12, Line 167: row is found in mu > I think the term 'version' here is a bit confusing, because even without th Done http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: Line 687: // Visitor which establishes the liveness of a row by applying deletes and reinserts. > +1 for LivenessVisitor Done Line 687: // Visitor which establishes the liveness of a row by applying deletes and reinserts. > we should rename this visitor to LivenessVisitor or DeleteReinsertVisitor o Done http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/tablet-test.cc File src/kudu/tablet/tablet-test.cc: PS12, Line 228: row > typo Done PS12, Line 229: ful > s/all/full/ Done Line 237: // Insert one row. > punctuation here and below in the comments Done Line 238: ASSERT_OK(this->InsertTestRow(&writer, 1, 0)); > ASSERT_OK() ? Done Line 239: > No need to check last_op_result(), InsertTestRow() already returns non-OK v Done PS12, Line 255: delet > typo: fourth Done -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
