You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/11/08 08:49:28 UTC
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4993
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Don't do UNDO garbage collection until after the REDO->UNDO transformation
Reasoning about what has been garbage collected and not while at the
same time generating UNDOs is tricky. Particularly taking REINSERTS
into account and multiple row versions merging (part of a follow up
patch).
This moves UNDO garbage collection to after the transformation and
simplifies things a bit, at the cost of some extra work.
Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_compaction.cc
3 files changed, 62 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/1
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Don't do UNDO garbage collection until after the REDO->UNDO transformation
Reasoning about what has been garbage collected and not while at the
same time generating UNDOs is tricky. Particularly taking REINSERTS
into account and multiple row versions merging (part of a follow up
patch).
This moves UNDO garbage collection to after the transformation and
simplifies things a bit, at the cost of some extra work.
Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Reviewed-on: http://gerrit.cloudera.org:8080/4993
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_compaction.cc
3 files changed, 72 insertions(+), 96 deletions(-)
Approvals:
David Ribeiro Alves: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4993
to look at the new patch set (#7).
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Don't do UNDO garbage collection until after the REDO->UNDO transformation
Reasoning about what has been garbage collected and not while at the
same time generating UNDOs is tricky. Particularly taking REINSERTS
into account and multiple row versions merging (part of a follow up
patch).
This moves UNDO garbage collection to after the transformation and
simplifies things a bit, at the cost of some extra work.
Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_compaction.cc
3 files changed, 72 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/7
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
PS6, Line 627: ahm
I think this comment would be more useful if it explained the intent a bit more - right now it's basically adding little value beyond the below if condition, which is pretty readable on its own. Maybe the comment could be 'Garbage collect rows that are deleted before the AHM'
Also do we not have to worry about the case where the redo_head is a delete, but there are further reviews after that which are reinserts? Are we assuming that this method is only called after the REDOs have been collapsed such that there are no REINSERTS in it? In that case, I think a CHECK that (*redo_head)->next() == nullptr is probably a good idea, and documenting it in the header
PS6, Line 679:
: // Convert the redos into undos. Any redos that are eligible for history GC
: // are applied and then thrown away (they don't generate undos).
is this comment still relevant?
Line 693: Mutation* current_undo;
does this need to be defined up here anymore? only seeing it used in one scope, but maybe I missed something
PS6, Line 699: \
nit: extra space
Line 724: CHECK(redo_delete == nullptr);
nit: CHECK_EQ
PS6, Line 727: undo_encoder.SetToDelete();
this is old code, but why do we need to use undo_encoder here at all, vs just passing redo_mut.changelist() on line 730?
Line 754: // Reset the UNDO head, losing all previous undos.
we've lost the 'num_rows_history_truncated' thing now. I guess you're figuring that since this history truncation thing is going away very soon, you didn't want to bother maintaining in this patch? I'm cool with that, just wanted to confirm the thinking.
http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:
PS6, Line 175: Mutation** redo_head
are the redos modified by this method? if so, should clarify in the doc. Otherwise, this should probably be a const Mutation* if we're just reading them
PS6, Line 187: // 'history_gc_opts': Options indicating whether row history should be
: // garbage-collected.
no longer an argument
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4993
to look at the new patch set (#4).
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Don't do UNDO garbage collection until after the REDO->UNDO transformation
Reasoning about what has been garbage collected and not while at the
same time generating UNDOs is tricky. Particularly taking REINSERTS
into account and multiple row versions merging (part of a follow up
patch).
This moves UNDO garbage collection to after the transformation and
simplifies things a bit, at the cost of some extra work.
Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_compaction.cc
3 files changed, 62 insertions(+), 88 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/4
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: 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-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4993/2/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 619: // If the row has a DELETE redo which is older than ahm, is_garbage_collected is set to true.
> Shouldn't this be in the header file? I also found this comment confusing,
Ping on the above comment.
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: 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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4993/2/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 619: // If the row has a DELETE redo which is older than ahm, is_garbage_collected is set to true.
Shouldn't this be in the header file? I also found this comment confusing, just reading this code without a lot of context you'd expect a variable named like that would imply that garbage collection took place, but we just run a check (which could be done outside of this function or in a different function).
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: 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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4993
to look at the new patch set (#6).
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Don't do UNDO garbage collection until after the REDO->UNDO transformation
Reasoning about what has been garbage collected and not while at the
same time generating UNDOs is tricky. Particularly taking REINSERTS
into account and multiple row versions merging (part of a follow up
patch).
This moves UNDO garbage collection to after the transformation and
simplifies things a bit, at the cost of some extra work.
Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_compaction.cc
3 files changed, 63 insertions(+), 90 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/6
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4993/2/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 619: // If the row has a DELETE redo which is older than ahm, is_garbage_collected is set to true.
> Shouldn't this be in the header file? I also found this comment confusing,
you mean the comment? done.
the variable name is the same as it was previously in ApplyMutationsAndGenerateUndos. it means that the row is garbage collected and shouldn't be output in the compaction output.
I moved the relevant comment from that method here (to the header of this method, that is).
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4993
to look at the new patch set (#3).
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Don't do UNDO garbage collection until after the REDO->UNDO transformation
Reasoning about what has been garbage collected and not while at the
same time generating UNDOs is tricky. Particularly taking REINSERTS
into account and multiple row versions merging (part of a follow up
patch).
This moves UNDO garbage collection to after the transformation and
simplifies things a bit, at the cost of some extra work.
Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
---
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_compaction.cc
3 files changed, 63 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/3
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: 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: Todd Lipcon <to...@apache.org>
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 8: Verified+1
unrelated flake OpenReadonlyFsITest.TestWriteAndVerify
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 724: break;
> The compiler complains about this:
hm, weird, I guess it used to work OK with NULL but maybe glog isn't C++11-savvy and doesn't know how to print nullptr. no big deal
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation
......................................................................
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
PS6, Line 627: ahm
> I think this comment would be more useful if it explained the intent a bit
Done
PS6, Line 679:
: // Convert the redos into undos. Any redos that are eligible for history GC
: // are applied and then thrown away (they don't generate undos).
> is this comment still relevant?
Done
Line 693: Mutation* current_undo;
> does this need to be defined up here anymore? only seeing it used in one sc
yeah, in a follow up I also use in the reinsert block, mind if I keep it here until then?
PS6, Line 699: \
> nit: extra space
Done
Line 724: CHECK(redo_delete == nullptr);
> nit: CHECK_EQ
The compiler complains about this:
error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const nullptr_t')
Is this something we usually do or am I missing something.
PS6, Line 727: undo_encoder.SetToDelete();
> this is old code, but why do we need to use undo_encoder here at all, vs ju
we don't, but this will change in a follow up patch so I'd rather leave it and fix it there, if thats ok
Line 754: // Reset the UNDO head, losing all previous undos.
> we've lost the 'num_rows_history_truncated' thing now. I guess you're figur
exactly.
http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:
PS6, Line 175: Mutation** redo_head
> are the redos modified by this method? if so, should clarify in the doc. Ot
Done
PS6, Line 187: // 'history_gc_opts': Options indicating whether row history should be
: // garbage-collected.
> no longer an argument
Done
--
To view, visit http://gerrit.cloudera.org:8080/4993
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes