You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2023/01/11 19:53:15 UTC

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19413


Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................

KUDU-3406 corrected estimate for ancient UNDO delta size

When looking into micro-benchmark results produced by the
$KUDU_HOME/src/kudu/scripts/benchmarks.sh script, I noticed that
dense_node-itest showed 9-fold increase in the number of blocks under
management.  Even if the test disables GC of the ancient UNDO deltas
(i.e. it runs with --enable_undo_delta_block_gc=false), that's not the
expected behavior.  It turned out the issue was in the way how
DeltaTracker::EstimateBytesInPotentiallyAncientUndoDeltas() operated: it
always treated a delta to be ancient if no stats were present.  So, if a
delta file was lazily loaded without stats being read, DeltaTracker
assumed all its deltas were ancient.  With the new behavior introduced
in 1556a353e, it lead to rowset merge compactions skipping the newly
generated UNDO deltas since the estimate reported 100% of the deltas
being ancient.

While this was not manifesting itself in real-world scenarios involving
some tangible amount of data being written, the artificial
"metadata only" workload exercised by dense_node-itest allowed the issue
to manifest itself.

This patch addresses the issue, introducing different estimate modes for
the method mentioned above and using proper modes in various contexts.
I verified that with this patch added, the benchmark based on
dense_node-itest now reports the number of blocks as it has been
reporting for the longest span of its history.  So I'm not adding any
new tests with this patch.

This is a follow-up to 1556a353e60c5d555996347cbd46d5e5a6661266.

Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
8 files changed, 56 insertions(+), 28 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/19413/1
-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 3: Verified+1

unrelated test failures in:
  * LeaderRebalancerTest.AddTserver
  * RaftConsensusITest.TestSlowFollower


-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 12 Jan 2023 22:20:40 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 17:00:34 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG@19
PS3, Line 19: lead
nit: led



-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 17:00:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 2: Code-Review+1

(4 comments)

Overall changes look good to me.

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@16
PS2, Line 16: So, if a
            : delta file was lazily loaded without stats being read, DeltaTracker
            : assumed all its deltas were ancient.
I noticed this once during my tests. I got following trace during that test:
+++
LOG_WITH_PREFIX(INFO) << Substitute(                        
    "compaction isn't runnable because of too much data in "
    "ancient UNDO deltas: $0 out of $1 total bytes",        
    ancient_undos_total_size, undos_total_size);
+++

I could not reproduce it later though.
Good that we found the root-cause.


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@19
PS2, Line 19: the
nit: for


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@25
PS2, Line 25: "metadata only"
Not a comment.

For my understanding, how does this type of workload help in manifesting the issue?


http://gerrit.cloudera.org:8080/#/c/19413/2/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

http://gerrit.cloudera.org:8080/#/c/19413/2/src/kudu/tablet/rowset.h@115
PS2, Line 115: OVERESTIMATE,
             :     UNDERESTIMATE,
nit: A one liner comment to explain in what case, one would choose this type.
e.g. For CompactRowSetsOp, why it would be UNDERESTIMATE and for UndoDeltaBlockGCOp, why an OVERESTIMATE.



-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 12 Jan 2023 07:22:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@16
PS2, Line 16: So, if a
            : delta file was lazily loaded without stats being read, DeltaTracker
            : assumed all its deltas were ancient.
> Right. That's exactly what happened during my testing. I had just generated
I see.  Yep, that makes sense.  If so, then certainly it means that more or less a regular workload might lead to a situation that dense_node-itest exposes.  That's good to know, thanks!


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@20
PS2, Line 20: the
> How about "those" ?
Done


http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG@19
PS3, Line 19: lead
> Good catch!
ack


http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG@19
PS3, Line 19: lead
> nit: led
Done


http://gerrit.cloudera.org:8080/#/c/19413/3/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

http://gerrit.cloudera.org:8080/#/c/19413/3/src/kudu/tablet/rowset.h@115
PS3, Line 115: delta 
> nit: delta is
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 02:56:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 11:07:17 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................

KUDU-3406 corrected estimate for ancient UNDO delta size

When looking into micro-benchmark results produced by the
$KUDU_HOME/src/kudu/scripts/benchmarks.sh script, I noticed that
dense_node-itest showed 9-fold increase in the number of blocks under
management.  Even if the test disables GC of the ancient UNDO deltas
(i.e. it runs with --enable_undo_delta_block_gc=false), that's not the
expected behavior.  It turned out the issue was in the way how
DeltaTracker::EstimateBytesInPotentiallyAncientUndoDeltas() operated: it
always treated a delta to be ancient if no stats were present.  So, if a
delta file was lazily loaded without stats being read, DeltaTracker
assumed all its deltas were ancient.  With the new behavior introduced
in 1556a353e, it led to rowset merge compactions skipping the newly
generated UNDO deltas since the estimate reported 100% of those deltas
being ancient.

While this was not detected by prior testing using various real-world
scenarios involving some tangible amount of data written, tracking
the history of stats emitted by dense_node-itest allowed to spot the
issue.

This patch addresses the issue, introducing different estimate modes for
the method mentioned above and using proper modes in various contexts.
I verified that with this patch added, the benchmark based on
dense_node-itest now reports the number of blocks as it has been
reporting for the longest span of its history.  So I'm not adding any
new tests with this patch.

This is a follow-up to 1556a353e60c5d555996347cbd46d5e5a6661266.

Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Reviewed-on: http://gerrit.cloudera.org:8080/19413
Tested-by: Kudu Jenkins
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
8 files changed, 73 insertions(+), 29 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Attila Bukor: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Ashwani Raina, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19413

to look at the new patch set (#3).

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................

KUDU-3406 corrected estimate for ancient UNDO delta size

When looking into micro-benchmark results produced by the
$KUDU_HOME/src/kudu/scripts/benchmarks.sh script, I noticed that
dense_node-itest showed 9-fold increase in the number of blocks under
management.  Even if the test disables GC of the ancient UNDO deltas
(i.e. it runs with --enable_undo_delta_block_gc=false), that's not the
expected behavior.  It turned out the issue was in the way how
DeltaTracker::EstimateBytesInPotentiallyAncientUndoDeltas() operated: it
always treated a delta to be ancient if no stats were present.  So, if a
delta file was lazily loaded without stats being read, DeltaTracker
assumed all its deltas were ancient.  With the new behavior introduced
in 1556a353e, it lead to rowset merge compactions skipping the newly
generated UNDO deltas since the estimate reported 100% of the deltas
being ancient.

While this was not detected by prior testing using various real-world
scenarios involving some tangible amount of data written, tracking
the history of stats emitted by dense_node-itest allowed to spot the
issue.

This patch addresses the issue, introducing different estimate modes for
the method mentioned above and using proper modes in various contexts.
I verified that with this patch added, the benchmark based on
dense_node-itest now reports the number of blocks as it has been
reporting for the longest span of its history.  So I'm not adding any
new tests with this patch.

This is a follow-up to 1556a353e60c5d555996347cbd46d5e5a6661266.

Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
8 files changed, 73 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/19413/3
-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@16
PS2, Line 16: So, if a
            : delta file was lazily loaded without stats being read, DeltaTracker
            : assumed all its deltas were ancient.
> I noticed this once during my tests. I got following trace during that test
The message should be there if the deltas are ancient -- that's not a problem.  The issue is when recent deltas are treated as if they were ancient.


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@19
PS2, Line 19: the
> nit: for
The meaning here is that those deltas were not picked up by the compaction.  As per New Oxford American Dictionary:
  * [with object] jump lightly over: the children used to skip the puddles.
  * [with object] omit (part of a book that one is reading, or a stage in a sequence that one is following): the video manual allows the viewer to skip sections he's not interested in | [no object] : she disliked him so much that she skipped over any articles that mentioned him.


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@25
PS2, Line 25: "metadata only"
> Not a comment.
I don't know whether the type of workload was crucial: this sentence simply describes the type of the workload used by the dense_node-itest.  I updated the sentence to avoid misinterpretations like that.


http://gerrit.cloudera.org:8080/#/c/19413/2/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

http://gerrit.cloudera.org:8080/#/c/19413/2/src/kudu/tablet/rowset.h@115
PS2, Line 115: OVERESTIMATE,
             :     UNDERESTIMATE,
> nit: A one liner comment to explain in what case, one would choose this typ
I added comments at the call sites of corresponding the methods/functions.



-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 12 Jan 2023 19:59:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/3//COMMIT_MSG@19
PS3, Line 19: lead
> nit: led
Good catch!

Ah! now I remember why I made that comment earlier (i.e. nit: for). I guess I misread it earlier as "leads"  and thought that "skipping for" would make more sense instead of "skipping the" later.



-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 17:44:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Ashwani Raina, Attila Bukor, Yifan Zhang, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19413

to look at the new patch set (#4).

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................

KUDU-3406 corrected estimate for ancient UNDO delta size

When looking into micro-benchmark results produced by the
$KUDU_HOME/src/kudu/scripts/benchmarks.sh script, I noticed that
dense_node-itest showed 9-fold increase in the number of blocks under
management.  Even if the test disables GC of the ancient UNDO deltas
(i.e. it runs with --enable_undo_delta_block_gc=false), that's not the
expected behavior.  It turned out the issue was in the way how
DeltaTracker::EstimateBytesInPotentiallyAncientUndoDeltas() operated: it
always treated a delta to be ancient if no stats were present.  So, if a
delta file was lazily loaded without stats being read, DeltaTracker
assumed all its deltas were ancient.  With the new behavior introduced
in 1556a353e, it led to rowset merge compactions skipping the newly
generated UNDO deltas since the estimate reported 100% of those deltas
being ancient.

While this was not detected by prior testing using various real-world
scenarios involving some tangible amount of data written, tracking
the history of stats emitted by dense_node-itest allowed to spot the
issue.

This patch addresses the issue, introducing different estimate modes for
the method mentioned above and using proper modes in various contexts.
I verified that with this patch added, the benchmark based on
dense_node-itest now reports the number of blocks as it has been
reporting for the longest span of its history.  So I'm not adding any
new tests with this patch.

This is a follow-up to 1556a353e60c5d555996347cbd46d5e5a6661266.

Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
8 files changed, 73 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/19413/4
-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19413 )

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@16
PS2, Line 16: So, if a
            : delta file was lazily loaded without stats being read, DeltaTracker
            : assumed all its deltas were ancient.
> The message should be there if the deltas are ancient -- that's not a probl
Right. That's exactly what happened during my testing. I had just generated load with all default flags, so there was no chance that those deltas could be ancient.


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@19
PS2, Line 19: the
> The meaning here is that those deltas were not picked up by the compaction.
Ah! my bad!
Thanks for the grammatical tidbit :-)


http://gerrit.cloudera.org:8080/#/c/19413/2//COMMIT_MSG@20
PS2, Line 20: the
How about "those" ?


http://gerrit.cloudera.org:8080/#/c/19413/3/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

http://gerrit.cloudera.org:8080/#/c/19413/3/src/kudu/tablet/rowset.h@115
PS3, Line 115: delta 
nit: delta is



-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 12:24:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3406 corrected estimate for ancient UNDO delta size

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Ashwani Raina, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19413

to look at the new patch set (#2).

Change subject: KUDU-3406 corrected estimate for ancient UNDO delta size
......................................................................

KUDU-3406 corrected estimate for ancient UNDO delta size

When looking into micro-benchmark results produced by the
$KUDU_HOME/src/kudu/scripts/benchmarks.sh script, I noticed that
dense_node-itest showed 9-fold increase in the number of blocks under
management.  Even if the test disables GC of the ancient UNDO deltas
(i.e. it runs with --enable_undo_delta_block_gc=false), that's not the
expected behavior.  It turned out the issue was in the way how
DeltaTracker::EstimateBytesInPotentiallyAncientUndoDeltas() operated: it
always treated a delta to be ancient if no stats were present.  So, if a
delta file was lazily loaded without stats being read, DeltaTracker
assumed all its deltas were ancient.  With the new behavior introduced
in 1556a353e, it lead to rowset merge compactions skipping the newly
generated UNDO deltas since the estimate reported 100% of the deltas
being ancient.

While this was not manifesting itself in real-world scenarios involving
some tangible amount of data being written, the artificial
"metadata only" workload exercised by dense_node-itest allowed the issue
to manifest itself.

This patch addresses the issue, introducing different estimate modes for
the method mentioned above and using proper modes in various contexts.
I verified that with this patch added, the benchmark based on
dense_node-itest now reports the number of blocks as it has been
reporting for the longest span of its history.  So I'm not adding any
new tests with this patch.

This is a follow-up to 1556a353e60c5d555996347cbd46d5e5a6661266.

Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
8 files changed, 57 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/19413/2
-- 
To view, visit http://gerrit.cloudera.org:8080/19413
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17bddae86f84792caf14fb1e11a6e1c0d7a92b56
Gerrit-Change-Number: 19413
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)