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] WIP: Don't output unobservable rows from the MemRowset
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4994
Change subject: WIP: Don't output unobservable rows from the MemRowset
......................................................................
WIP: Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1
Delete Row at 3
Flush - Ghost 1 now lives in RS1
Insert Row at 3
Delete Row at 3
Flush - Ghost 2 now lives in RS2
Major delta compact RS1 - GC all before 3
Major delta compact RS2 - GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
WIP as this is missing a small directed test.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction.cc
1 file changed, 45 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/1
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] Don't output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 11:
regarding the case you suggested, yeah, v1 would be chosen to be the most recent one and to point to v2 as the previous_ghost, but that wouldn't matter since they can't be observed since they are behind the ahm.
fuzz-itest doesn't do garbage collection iirc. maybe thats something we can improve in the future?
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 11
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 17: Code-Review+1
(5 comments)
just a rebase, keeping todd's +1
http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
Line 618: {
> Is it important that we do not roll?
yeah, we want a single rowset, not that these rows would fill one, but this way we're sure and we dont have to make assertion on the number of the resulting rowsets.
http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 113: // Get the latest mutation.
> Consider writing this like:
Done.
Its unlikely that the we have both a redo head and that it's timesatmp is the same as the insertion timestamp, right.
If for nothing else I think it has documentation value.
Line 114: const Mutation* latest = input_row.redo_head;
> nit: punctuation
Done
Line 117: latest->timestamp() == insertion_timestamp) {
> similarly, consider writing this like:
Done
Line 134: block->resize(next_row_index);
> Maybe better written as:
Done
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 17
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 output unobservable rows from the MemRowset
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 output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
The point is that we can't trust UNDOs to break ties, in terms of
recency, between two ghosts as they might have been removed by
delta compactions. However we can always trust REDO delete/reinsert
to remain.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
FuzzTest and TestRandomAccess would fail for the following patch
(KUDU-237 (part 2)) without this change.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Reviewed-on: http://gerrit.cloudera.org:8080/4994
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 185 insertions(+), 62 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, but someone else must approve
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 18
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/11/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
PS11, Line 121: if (PREDICT_FALSE(
: input_row.redo_head != nullptr
: && insertion_timestamp.CompareTo(input_row.redo_head->timestamp()) == 0)) {
: // Get the latest mutation
: const Mutation* latest = input_row.redo_head;
: AdvanceToLastInListConst(&latest);
: if (latest->changelist().is_delete()
: && insertion_timestamp.CompareTo(latest->timestamp()) == 0) {
: iter_->Next();
: continue;
: }
> isn't this specifically only catching the case where the initial insert was
delta compactions don't transform reinsert/delete redos into undos, so those delete/reinsert/delete mutations would never get gcd until a regular compaction happened at which point they would have served the purpose of establishing which row is the most recent.
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 11
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 output unobservable rows from the MemRowset
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 13:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:
Line 618: FlushMRSAndReopenNoRoll(*mrs, schema_, &rs2);
Is it important that we do not roll?
http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 113: && insertion_timestamp.CompareTo(input_row.redo_head->timestamp()) == 0)) {
Consider writing this like:
if (PREDICT_FALSE(input_row.redo_head != nullptr &&
input_row.redo_head->timestamp() == insertion_timestamp)) {
I also wonder if PREDICT_FALSE is really needed here.
Line 114: // Get the latest mutation
nit: punctuation
Line 117: if (latest->changelist().is_delete()
similarly, consider writing this like:
if (latest->changelist().is_delete() &&
latest->timestamp() == insertion_timestamp) {
Line 134: if (PREDICT_FALSE(num_in_block != next_row_index)) {
Maybe better written as:
if (PREDICT_FALSE(next_row_index < num_in_block)) {
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 13
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 output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#6).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1
Delete Row at 3
Flush - Ghost 1 now lives in RS1
Insert Row at 3
Delete Row at 3
Flush - Ghost 2 now lives in RS2
Major delta compact RS1 - GC all before 3
Major delta compact RS2 - GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 147 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/6
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 6
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 output unobservable rows from the MemRowset
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/6//COMMIT_MSG
Commit Message:
PS6, Line 18: Delete Row at 3
: Flush - Ghost 1 now lives in RS1
: Insert Row at 3
: D
not following how this part of the sequence could happen. A single operation (at a given timestamp) should land entirely before or after the flush. If we're splitting a single op across the flush that seems like something else might be wrong?
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Don't output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/9//COMMIT_MSG
Commit Message:
Line 28: for the row.
> Yea, I agree it's impossible to establish which one is older, but you can o
yeah, I had considered that (both are deleted and really close to the ahm so likely not observable, which is why I didn't come up with a user observable scenario) but honestly I really like the check that makes sure we can always establish which version of a row comes first. Specially as we historically miss some edge cases. So it was between giving that up or doing something super silly (like making sure both rows are exactly in this scenario at which point why would they be in the compaction input anyway) or adding this. I think adding this is unimpactful since the row is unobservable anyway.
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: 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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 12:
btw fuzz-itest does a really good job at finding these corner cases. having gc there would likely be very useful to catch corner cases such as the one this patch addresses, who knows how many more we have even before this patch series. but tbh I don't have the bandwidth to do it now
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 12
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/11/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
PS11, Line 63: // This can only be applied to mutation lists that are not in a memory store.
> can you explain why?
actually this disappeared in the next patch so I removed it.
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 11
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 output unobservable rows from the MemRowset
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 13:
>
> DRS 1:
> undo: del @ 2
> base: v1
> redo delta: del @3
>
> DRS 2:
> base: v2
>
>
> ----------------
> flush and compact
> ---------------
>
> ?
I think in this case you would still have a del UNDO @ 3 in DRS 2, so you could reason that at the same TS a del UNDO in a higher RS comes after a del REDO in a lower RS. Right?
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 13
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 output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#8).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is delete from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 177 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/8
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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>
[kudu-CR] Don't output unobservable rows from the MemRowset
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/9//COMMIT_MSG
Commit Message:
Line 28: for the row.
still not 100% sure I understand the issue here. Here's what I think the state would look like:
DRS1 (original flushed MRS):
undos: delete @ 1
base: v1
redos: delete @ 3
MRS2 in memory:
undo: delete @ 3
base: v2
redo: delete @ 3
redo: reinsert v3 @ 3
redo: delete @ 3
After flushing MRS2, becomes DRS2:
undo: delete @ 3
undo: reinsert v2 @ 3
undo: delete @ 3
base: v3
redo: delete @ 3
<-- gc runs, AHM = 3-->
compaction input:
DRS1:
base: v1
redos: delete @ 3
DRS2:
undo: delete @ 3
undo: reinsert v2 @ 3
undo: delete @ 3
base: v3
redo: delete @ 3
any scan at time=2 would be rejected (< AHM).
any scan at time=3 would know not to show the row regardless which order the compaction merges these deltas, right?
The included test seems like a good unit test that the change is implemented as described, but would be nicer to have a scenario using the higher-level APIs that can recreate this issue to illustrate it more clearly
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: 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 output unobservable rows from the MemRowset
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 15
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4994
to look at the new patch set (#16).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
The point is that we can't trust UNDOs to break ties, in terms of
recency, between two ghosts as they might have been removed by
delta compactions. However we can always trust REDO delete/reinsert
to remain.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
FuzzTest and TestRandomAccess would fail for the following patch
(KUDU-237 (part 2)) without this change.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 185 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/16
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 16
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 output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#9).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 177 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/9
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Don't output unobservable rows from the MemRowset
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/9//COMMIT_MSG
Commit Message:
Line 28: for the row.
> (unrelated nit) Isn't it more like:
Yea, I agree it's impossible to establish which one is older, but you can only get to this situation when the AHM has moved above 3 (such that you are removing the UNDO @ 3). Given that, no one can ever read this base data, so it doesn't actually matter which one you pick, does it?
Was thinking an end-to-end test that does the above sequence at a tablet level would be a good way to show what happens without the patch and why it causes a problem (some incorrect scan results or whatever)
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: 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] WIP: Don't output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#2).
Change subject: WIP: Don't output unobservable rows from the MemRowset
......................................................................
WIP: Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1
Delete Row at 3
Flush - Ghost 1 now lives in RS1
Insert Row at 3
Delete Row at 3
Flush - Ghost 2 now lives in RS2
Major delta compact RS1 - GC all before 3
Major delta compact RS2 - GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
WIP as this is missing a small directed test.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction.cc
1 file changed, 46 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/2
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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>
[kudu-CR] Don't output unobservable rows from the MemRowset
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 17: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 17
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 output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#15).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
The point is that we can't trust UNDOs to break ties, regarding
recency, between two ghosts as they might have been removed by
delta compactions. However we can always trust REDO delete/reinsert
to remain.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 186 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/15
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 15
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/9//COMMIT_MSG
Commit Message:
Line 28: for the row.
> still not 100% sure I understand the issue here. Here's what I think the st
(unrelated nit) Isn't it more like:
DRS1 (original flushed MRS):
undos: delete @ 1
base: v1
redos: delete @ 3
MRS2 in memory:
undo: delete @ 3
base: v2
redo: delete @ 3
After flushing MRS2, becomes DRS2:
undo: delete @ 3
base: v3
redo: delete @ 3
right, my point was not about the scan. it spawned from the reinsert part 2 patch and it was about the fact that the rows get to a state where it's indistinguishable which one is the older version.
Say that after two delta compactions with garbage collection we get:
DRS1:
base: v1
redos: delete @ 3
DRS2:
base: v3
redo: delete @ 3
It's impossible to establish which one is oldest
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: 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 output unobservable rows from the MemRowset
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 11:
I'm still a little worried about the "manufactured" nature of this test. Is this scenario produced by any higher level test? eg the fuzz tests, etc?
Do we have coverage for the other similar scenario:
initial state
--------------
DRS 1:
undo: delete @2
base: v1
MRS empty
-------
txn @3: delete, insert v2
----------
DRS 1:
undo: del @ 2
base: v1
redo delta: del @3
MRS:
undo: del @3
base: v2
-----
flush MRS, AHM = 5
-------
DRS 1:
undo: del @ 2
base: v1
redo delta: del @3
DRS 2:
base: v2
----------------
flush and compact
---------------
?
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 11
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 output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#12).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 168 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/12
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 12
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4994/6//COMMIT_MSG
Commit Message:
PS6, Line 18: Flush -> Row now lives in RS1
: Delete Row at 3 -> Row is now a ghost in RS1
: Insert Row at 3 -> Row goes in the MRS again
: D
> not following how this part of the sequence could happen. A single operatio
yeah, I was trying to simplify the sequence and apparently simplified it to a point where it's impossible to happen :) fixed this and also illustrated the sequence in the test.
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: Yes
[kudu-CR] Don't output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#11).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 178 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/11
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 11
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 output unobservable rows from the MemRowset
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/4994
to look at the new patch set (#14).
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Don't output unobservable rows from the MemRowset
In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.
The sequence is as follows:
Insert Row at 1 -> Row goes in the MRS
Flush -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3
If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.
This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.
Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 88 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/14
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 14
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 output unobservable rows from the MemRowset
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4994/11/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
PS11, Line 63: // This can only be applied to mutation lists that are not in a memory store.
can you explain why?
PS11, Line 121: if (PREDICT_FALSE(
: input_row.redo_head != nullptr
: && insertion_timestamp.CompareTo(input_row.redo_head->timestamp()) == 0)) {
: // Get the latest mutation
: const Mutation* latest = input_row.redo_head;
: AdvanceToLastInListConst(&latest);
: if (latest->changelist().is_delete()
: && insertion_timestamp.CompareTo(latest->timestamp()) == 0) {
: iter_->Next();
: continue;
: }
isn't this specifically only catching the case where the initial insert was deleted in the same transaction?
What about a case where we insert at t=1, then delete at t=2, then insert/delete at t=3? that scenario doesn't cause problems at compaction? even after various cases of major GC?
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 11
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 output unobservable rows from the MemRowset
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Don't output unobservable rows from the MemRowset
......................................................................
Patch Set 6: Verified+1
unrelated flake DebugUtilTest.TestSignalStackTrace
--
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
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: No