You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2021/09/27 16:33:08 UTC
[kudu-CR] WIP KUDU-3318 [LBM] Runtime compact log container metadata
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17871
Change subject: WIP KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
WIP KUDU-3318 [LBM] Runtime compact log container metadata
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 266 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/1
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 9: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@202
PS9, Line 202: F
nit: align this with the "("
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@495
PS9, Line 495: enum class ProcessRecordType {
: kReadOnly, // Read records only.
: kReadAndUpdate, // Read records and update container's statistic.
: };
nit: can you add a comment mentioning why this is used, so users can better understand when to use it? E.g.
"Whether to update internal counters based on processed records. This may be useful to avoid recomputing container statistics during operations that don't change them, e.g. compacting container metadata."
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@629
PS9, Line 629: if (static_cast<double>(live_blocks()) / total_blocks() >=
: FLAGS_log_container_live_metadata_before_compact_ratio) {
: return false;
: }
Considering rewriting using multiplication to avoid the costlier division
https://stackoverflow.com/questions/17883240/is-multiplication-faster-than-float-division
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@2647
PS9, Line 2647: std::lock_guard<simple_spinlock> l(lock_);
Now that 'all_containers_by_name_' isn't referenced here, we don't need this lock
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 26 Oct 2021 00:36:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Reviewed-on: http://gerrit.cloudera.org:8080/17871
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 321 insertions(+), 71 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] WIP KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#2).
Change subject: WIP KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
WIP KUDU-3318 [LBM] Runtime compact log container metadata
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 270 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/2
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#4).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 296 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/4
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#10).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 316 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/10
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#11).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 316 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/11
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#12).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 321 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/12
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 12: Verified+1 Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 23 Mar 2022 05:53:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 11: Code-Review+2
(4 comments)
Thanks for revving this! Just more nits, otherwise LGTM.
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@89
PS11, Line 89: DEFINE_uint64(log_container_metadata_max_size, 0,
: "Maximum size (soft) of a log container's metadata. Use 0 for "
: "no limit.");
Empirically, and for posterity, I'm curious what values have been useful in resolving the space issues you've seen in your clusters. I imagine this would be directly correlated with the --log_container_max_size configuration, since the size of each metadata record is fairly constant, though with some wiggle room for deletes.
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@123
PS11, Line 123: at startup
nit: "..., or at runtime, based on the configuration of --log_container_metadata_size_before_compact_ratio."
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@127
PS11, Line 127: "Desired ratio of block metadata size of --log_container_metadata_max_size. "
: "If a container's metadata file size exceed --log_container_metadata_max_size * "
: "--log_container_metadata_size_before_compact_ratio, the container is going "
: "to be compact at runtime."
nit: IMO it isn't clear from this what the behavior is, especially in considering log_container_live_metadata_before_compact_ratio.
How about something like,
"Desired portion of --log_container_metadata_max_size container metadata must consume before metadata is considered for compaction. If a container's metadata file exceeds --log_container_metadata_max_size * --log_container_metadata_size_before_compact_ratio, and the number of live blocks falls below --log_container_live_metadata_before_compact_ratio, the container's metadata will be compacted."
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@868
PS11, Line 868: // Check again is necessary, metadata may has been compacted very before.
nit: maybe
"Check again, in case the metadata was compacted while we were waiting."
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 21 Mar 2022 22:21:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@208
PS3, Line 208: enabli
> nit: "when enabling"
Done
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@576
PS3, Line 576: // Try lock before reading metadata offset, consider it not full if lock failed.
: if (!metadata_lock_.try_lock()) {
: return false;
: }
> Does this mean that if a metadata file is getting written to a lot, we may
Yes, it will cause metadata exceed it's size limitation, but it's not a serious issue, we consider it's just a soft limit.
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@621
PS3, Line 621: bool ShouldCompact() const {
> nit: maybe call this ShouldCompact()?
Done
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@628
PS3, Line 628: if (static_
> nit: typically when we need both a locked and an unlocked version, that tak
Done
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@864
PS3, Line 864: vector<BlockRecordPB> records = SortRecords(live_block_records);
> In the event of a disk failure, rather than crashing, perhaps we should set
Good, we can use SetReadOnly in this case.
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@2632
PS3, Line 2632: arbage at startup time (in the event that we crashed just before the
> Rather than looking up the container reference, could we use scoped_refptr<
Done
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 08:12:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 10: Code-Review+1
(1 comment)
Basically LGTM, though still would like to see this run on a real cluster. Also curious (and maybe it's worth looking at some LBM test timings for it) whether the new lock affects performance.
http://gerrit.cloudera.org:8080/#/c/17871/10/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/10/src/kudu/fs/log_block_manager.cc@1339
PS10, Line 1339: shared_lock<RWMutex> l(metadata_compact_lock_);
Given the new feature is off by default, I'm curious whether the addition of this lock has a noticeable effect on performance. I suppose concurrent access of metadata would be rare, but it's still worth thinking about.
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Nov 2021 01:48:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#6).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 297 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/6
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#9).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 314 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/9
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/17871/3//COMMIT_MSG
Commit Message:
PS3:
> Didn't try on cluster yet, I will do more test next
Cool, looking forward to seeing if it helps! I also wonder whether this will help reduce startup times at all, since mostly-deleted containers will require fewer cycles to read through when starting up.
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager-test.cc@1253
PS3, Line 1253: 1 << 15;
nit: prefer to be more explicit with these values (e.g. 32 * 1024), optimizing tests for readability over micro-optimizations likes this
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager-test.cc@1254
PS3, Line 1254: mt_create_and_delete_blocks();
nit: wrap in NO_FATALS() so the error messages point to this line number if it fails. Same elsewhere
http://gerrit.cloudera.org:8080/#/c/17871/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/5/src/kudu/fs/log_block_manager.cc@864
PS5, Line 864: live_block_records
We should std::move() this to avoid a copy.
http://gerrit.cloudera.org:8080/#/c/17871/5/src/kudu/fs/log_block_manager.cc@2812
PS5, Line 2812: vector<BlockRecordPB> records = LogBlockContainer::SortRecords(live_block_records);
We should std::move() this to avoid a copy.
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:31:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#5).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 297 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/5
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@89
PS11, Line 89: DEFINE_uint64(log_container_metadata_max_size, 0,
: "Maximum size (soft) of a log container's metadata. Use 0 for "
: "no limit.");
> Empirically, and for posterity, I'm curious what values have been useful in
It may be correlated with --log_container_max_size in one usage scenario, but in different scenario, the correlation may be very different, the width fo the table, the delete rate in a container, may take effect, so it maybe difficult to calculate meaningful --log_container_max_size configurations for every scenarion, but set a same constant --log_container_metadata_max_size configuration for these clusters may help.
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@123
PS11, Line 123: at startup
> nit: "..., or at runtime, based on the configuration of --log_container_met
Done
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@127
PS11, Line 127: "Desired ratio of block metadata size of --log_container_metadata_max_size. "
: "If a container's metadata file size exceed --log_container_metadata_max_size * "
: "--log_container_metadata_size_before_compact_ratio, the container is going "
: "to be compact at runtime."
> nit: IMO it isn't clear from this what the behavior is, especially in consi
Done
http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@868
PS11, Line 868: // Check again is necessary, metadata may has been compacted very before.
> nit: maybe
Done
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 23 Mar 2022 02:41:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17871/10/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/10/src/kudu/fs/log_block_manager.cc@1339
PS10, Line 1339: // It is invalid to punch a zero-size hole.
> Given the new feature is off by default, I'm curious whether the addition o
After upgrading partial tservers of some real clusters to include this patch, comparing write/compact related metrics of tservers, I didn't see any noticeable effect on performance.
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 14 Mar 2022 09:28:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#3).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 281 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/3
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#8).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 284 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/8
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 12:
Thanks for the contribution!
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 23 Mar 2022 05:54:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17871/3//COMMIT_MSG
Commit Message:
PS3:
> Have you tried deploying this on a cluster? Curious if you were able to see
Didn't try on cluster yet, I will do more test next
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 09:28:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager-test.cc@1253
PS3, Line 1253: 32 * 102
> nit: prefer to be more explicit with these values (e.g. 32 * 1024), optimiz
Done
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager-test.cc@1254
PS3, Line 1254: NO_FATALS(mt_create_and_delete
> nit: wrap in NO_FATALS() so the error messages point to this line number if
Done
http://gerrit.cloudera.org:8080/#/c/17871/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/5/src/kudu/fs/log_block_manager.cc@864
PS5, Line 864: std::move(live_blo
> We should std::move() this to avoid a copy.
Done
http://gerrit.cloudera.org:8080/#/c/17871/5/src/kudu/fs/log_block_manager.cc@2812
PS5, Line 2812: vector<BlockRecordPB> records = LogBlockContainer::SortRecords(std::move(live_block_records));
> We should std::move() this to avoid a copy.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Oct 2021 02:49:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 9:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@202
PS9, Line 202: F
> nit: align this with the "("
Done
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@495
PS9, Line 495: enum class ProcessRecordType {
: kReadOnly, // Read records only.
: kReadAndUpdate, // Read records and update container's statistic.
: };
> nit: can you add a comment mentioning why this is used, so users can better
Thanks, done
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@629
PS9, Line 629: if (static_cast<double>(live_blocks()) / total_blocks() >=
: FLAGS_log_container_live_metadata_before_compact_ratio) {
: return false;
: }
> Considering rewriting using multiplication to avoid the costlier division
Done
http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@2647
PS9, Line 2647: std::lock_guard<simple_spinlock> l(lock_);
> Now that 'all_containers_by_name_' isn't referenced here, we don't need thi
Done
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Oct 2021 15:50:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17871/3//COMMIT_MSG
Commit Message:
PS3:
Have you tried deploying this on a cluster? Curious if you were able to see it solve the high disk usage issue you were seeing earlier.
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@208
PS3, Line 208: enable
nit: "when enabling"
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@576
PS3, Line 576: // Try lock before reading metadata offset, consider it not full if lock failed.
: if (!metadata_lock_.try_lock()) {
: return false;
: }
Does this mean that if a metadata file is getting written to a lot, we may repeatedly consider it not full, even though it is?
+1 to the idea of adding metadata size, if that helps us avoid this.
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@621
PS3, Line 621: bool check_compact_condition(bool lock) const {
nit: maybe call this ShouldCompact()?
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@628
PS3, Line 628: if (lock) {
nit: typically when we need both a locked and an unlocked version, that takes the form of
T DoSomethingUnlocked() {
// maybe DCHECK that the lock is held
// ... does something
}
T DoSomething() {
Lock l(lock_);
DoSomethingUnlocked();
}
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@864
PS3, Line 864: // However, we're hosed if we can't open the new metadata file.
In the event of a disk failure, rather than crashing, perhaps we should set some flag in the container to indicate to appenders that the container is bad?
http://gerrit.cloudera.org:8080/#/c/17871/3/src/kudu/fs/log_block_manager.cc@2632
PS3, Line 2632: LogBlockContainerRefPtr c = FindOrDie(all_containers_by_name_, lb->container()->ToString());
Rather than looking up the container reference, could we use scoped_refptr<T>& operator=(T* p) to generate a scoped refptr? e.g. scoped_refptr<LogBlockContainer> self(lb->container());
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 29 Sep 2021 07:54:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17871
to look at the new patch set (#7).
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata
......................................................................
KUDU-3318 [LBM] Runtime compact log container metadata
In commit 49abc3a6788bc29c8c023cafff0f0955142a2409, we have added
a logic to make log container metadata file to be full, then no
more CREATE type blocks will be appended to this container. However,
the metadata will still consuming disk space even if it's in a very
low live ratio.
This patch adds a method to compact metadata when found it's in low
live ratio when append DELETE type blocks to it.
Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 288 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/17871/7
--
To view, visit http://gerrit.cloudera.org:8080/17871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6
Gerrit-Change-Number: 17871
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>