You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2017/09/28 04:30:21 UTC
[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8162
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 4]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operation.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punch works as expected by checking the vaule of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 244 insertions(+), 115 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/1
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.h@296
PS9, Line 296: // blocks were already deleted, e.g encountered 'NotFound' error during removal.
> encountered
Done
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@2064
PS9, Line 2064: }
> Can this be const auto& ?
I used std::move() at L2089, so it seems not good to use const auto& here as Tidy pointed out.
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@2075
PS9, Line 2075:
: lb->container()->BlockDeleted(lb);
> Only do CloneAndPrepend() if !s.ok().
Done
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@2126
PS9, Line 2126: if (ContainsKey(failed_dirs, uuid_idx)) {
: LOG_EVERY_N(INFO, 10) << Substitute("Block $0 is in a failed directory; not deleting",
: block_id.ToString());
: return Status::IOError("Block is in a failed directory");
: }
: }
> Now that we're passing 'lb' back to RemoveLogBlocks, we could consolidate a
Done
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:02:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Reviewed-on: http://gerrit.cloudera.org:8080/8162
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
9 files changed, 436 insertions(+), 189 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#7).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
9 files changed, 424 insertions(+), 184 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/7
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 11:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@260
PS10, Line 260: (*prototyp
> I think it'd be better to do a FindOrDie here to be more explicit about wha
It seems MetricEntity does not have FindOrDie, so dereference here.
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@266
PS10, Line 266: int expected_value, const MetricPrototype* prototype) {
> likewise
Done
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@1069
PS10, Line 1069: TEST_F(LogBlockManagerTest, TestMisalignedBlocksFuzz) {
> Are these explicit blocks necessary for correctness, or is it just to add a
Yeah, it is necessary for correctness.
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc@231
PS10, Line 231: ction>& tr
> should this be committed?
As you noticed, I think using 'destructed' is more accurate.
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc@1244
PS10, Line 1244:
> What is the reason to do this work in the destructor instead of in CommitDe
Done
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 24 Oct 2017 23:05:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 25 Oct 2017 18:48:45 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@88
PS5, Line 88: // Block manager metrics.
> Nit: separate with a space.
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@257
PS5, Line 257: static void CheckGaugeMetric(const scoped_refptr<MetricEntity>& entity,
> It's getting too hard to understand what each call to this means. How about
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@340
PS5, Line 340: {1, &METRIC_log_block_manager_blocks_under_management},
> Would it be interesting to test the metrics after this but before the trans
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.h@308
PS5, Line 308: scoped_refptr<internal::LogBlock>* lb);
> Doesn't LogBlock have a block ID member? Why do you need the first argument
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@130
PS5, Line 130: kudu::MetricUnit::kLogBlockContainers,
> The unit is actually "holes". You may have to add a new MetricUnit.
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@131
PS5, Line 131: "Number of log block containers");
> "Number of holes punched since service start"
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1211
PS5, Line 1211: ionTransaction,
> deleted_block_map_
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1219
PS5, Line 1219:
> This is only used in one place, so don't bother with a typedef.
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1224
PS5, Line 1224: // Add the given block that needs to be deleted to 'deleted_interval_map_',
> These aren't blocks though, right? It's more like a 'deleted_interval_map_'
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1253
PS5, Line 1253: container->ExecClosure(Bind(&LogBlockContainer::ContainerDeletionAsync,
> What should we do if this fails? Maybe we should coalesce in CommitDeletedB
Yeah, we should launch the hole punch operation for this container and log it in case of failure here. But this scenario should never happen, as block length should >= 0 (the only failure case for CoalesceIntervals()). So I think it should be fine to keep it here.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1257
PS5, Line 1257: }
> IIUC, this arg is only here because friendship with a free function is toug
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1266
PS5, Line 1266:
> Oh I see, you moved the checks from NewDeletionTransaction to here. I'd pre
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1269
PS5, Line 1269:
> Nit: missing a space here
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1270
PS5, Line 1270: if (!s.ok()) {
: if (first_failure.ok()) first_failure = s;
: } else {
: deleted->emplace_back(lb->block_id());
: // Register the block to be hole punched if metadata recording
: // is successful.
: lb->RegisterDeletion(transaction);
: AddBlock(lb);
:
: if (lbm_->metrics_) {
: lbm_->metrics_->generic_metrics.total_blocks_deleted->Increment();
: }
: }
: }
:
> Having one class grab the internal lock of another class is a code smell an
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435
PS5, Line 1435: Status LogWritableBlock::AppendV(ArrayView<const Slice> data) {
> Hmm, I'd really like to eliminate DeleteBlock() altogether, as that would s
Sorry, I do not quite follow why do we need to remove DoCloseBlocks() here in this patch?
Yeah, there is no reason stop us from removing DeleteBlock() here. I think I probably just missed that.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1897
PS5, Line 1897:
> Nit: it's not a container in this code path.
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2604
PS5, Line 2604:
> belonging
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2610
PS5, Line 2610: internal::LogBlockContainer* container = FindPtrOrNull(containers_by_name,
:
> It seems like these two are always called together. Perhaps they can be com
Yeah, previously I combined them. But you pointed out that there are circle reference between LogBlockDeletionTransaction and LogBlock in LogBlock::RegisterDeletion. So I separate it out. Do you think that is still a problem? Or you mean combine them in another way?
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 16 Oct 2017 06:16:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Oct 2017 22:06:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#6).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.h
8 files changed, 425 insertions(+), 184 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/6
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#8).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
9 files changed, 424 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/8
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#10).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
9 files changed, 432 insertions(+), 191 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/10
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208
PS3, Line 1208: public:
: explicit LogBlockDeletionTransaction(LogBlockManager* lbm)
: : lbm_(lbm) {
: }
> I agree with you it will be more accurate. But the only issue here is doing
The hole punches are queued in the data directory's thread pool, so if you can get to that, you can Wait() on it which will wait for them all to finish.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1286
PS3, Line 1286: } else {
> Updated to make BlockCreationTransaction and BlockDeletionTransaction pure
Ah, yeah I guess that's true (about merging steps #2 and #3).
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 00:18:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 4:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@13
PS3, Line 13: It also adds a new metric 'holes_punched' in log block manager to track
> Let's also add a new counter for 'blocks_deleted', which will track the tot
Done
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@14
PS3, Line 14: operation
> operations
Done
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17
PS3, Line 17:
> value
Done
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17
PS3, Line 17:
> punching
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h@323
PS3, Line 323:
> In part 3 I left a comment about how it'd be nice for these transaction cla
Right, I agree with it and updated part3.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc@347
PS3, Line 347: FLAGS_log_container_max_size = 10LU * 1024 * 1024 * 1024;
> Add a using:: for this.
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@128
PS3, Line 128: METRIC_DEFINE_counter(server, log_block_manager_holes_punched,
> Because the number of holes punched only ever increases, this should be a c
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@129
PS3, Line 129: "Number of Holes Punched",
> Update.
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@130
PS3, Line 130: kudu::MetricUnit::kLogBlockContainers,
> Update.
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@131
PS3, Line 131: "Number of hole punching operations");
> Update.
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@182
PS3, Line 182:
> Please separate from the previous two metrics with a blank line, since it's
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@229
PS3, Line 229:
> with
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@230
PS3, Line 230:
> is
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@246
PS3, Line 246:
> with which this block has been registered.
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208
PS3, Line 1208: public:
: explicit LogBlockDeletionTransaction(LogBlockManager* lbm)
: : lbm_(lbm) {
: }
> For this to be more accurate:
I agree with you it will be more accurate. But the only issue here is doing so will make LogBlockManagerTest::MetricsTest hard to work, because we don't know when the hole punching operation actually takes place?
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1224
PS3, Line 1224: // Block <offset, offset + length> pair.
> Would be better if:
Done
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1286
PS3, Line 1286: } else {
> The call sequence for deleting blocks is long and involves a lot of class t
Updated to make BlockCreationTransaction and BlockDeletionTransaction pure virtual, and added another patch to force all deletions through BlockDeletionTransaction. But I think for the second recommendation, we probably cannot merge #2 with #3? Since we need to do in-memory deletions separately from #3 to have the transaction goes out of scope (free all the references to the deleted LogBlocks).
Given the current updates, now LogBlockDeletionTransaction::CommitDeletedBlocks doesn't have back-and-forth call chain. It only calls LogBlockManager::RemoveLogBlock and LogBlock::RegisterDeletion separately.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@2550
PS3, Line 2550:
: // Technically we've "repaired" the inconsistency if the truncation
> Remove/update.
Done
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 05 Oct 2017 17:25:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#3).
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 4]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operation.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punch works as expected by checking the vaule of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 254 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/3
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 5:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@88
PS5, Line 88: //Block manager metrics.
Nit: separate with a space.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@257
PS5, Line 257: static void CheckLogMetrics(const scoped_refptr<MetricEntity>& entity,
It's getting too hard to understand what each call to this means. How about:
1. Add a function that given two sets (one of counter prototypes and one of gauge prototypes), tests them for the value 0.
2. Add a function that tests an individual counter prototype for a specific value.
3. Add a function that tests an individual gauge prototype for a specific value.
Maybe that will be more clear without increasing the size of MetricsTest too much?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@340
PS5, Line 340: ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted));
Would it be interesting to test the metrics after this but before the transaction goes out of scope?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.h@308
PS5, Line 308: const scoped_refptr<internal::LogBlock>& lb);
Doesn't LogBlock have a block ID member? Why do you need the first argument?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@130
PS5, Line 130: kudu::MetricUnit::kLogBlockContainers,
The unit is actually "holes". You may have to add a new MetricUnit.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@131
PS5, Line 131: "Number of hole punching operations");
"Number of holes punched since service start"
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1211
PS5, Line 1211: deleted_block_map
deleted_block_map_
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1219
PS5, Line 1219: // Map used to aggregate BlockInterval instances across containers.
This is only used in one place, so don't bother with a typedef.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1224
PS5, Line 1224: BlockDataByContainerMap deleted_block_map_;
These aren't blocks though, right? It's more like a 'deleted_interval_map_'.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1253
PS5, Line 1253: CoalesceIntervals<int64_t>(&entry.second);
What should we do if this fails? Maybe we should coalesce in CommitDeletedBlocks() instead?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1257
PS5, Line 1257: container->block_manager()->metrics(),
IIUC, this arg is only here because friendship with a free function is tough, right? What if ContainerDeletionAsync were a LogBlockContainer method? Would that address this issue?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1266
PS5, Line 1266: CHECK(!lbm_->read_only_);
Oh I see, you moved the checks from NewDeletionTransaction to here. I'd prefer the other way around; better to catch a misuse of a read-only fsmanager early.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1269
PS5, Line 1269: >
Nit: missing a space here
Also, why do you need this pair? The LogBlock has a block ID in it.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1270
PS5, Line 1270: {
: std::lock_guard<simple_spinlock> l(lbm_->lock_);
: for (const auto &block_id : deleted_blocks_) {
: scoped_refptr<LogBlock> lb;
: Status s = lbm_->RemoveLogBlockUnlocked(block_id, &lb);
: // If we get NotFound, then the block was already deleted.
: if (!s.ok() && !s.IsNotFound()) {
: if (first_failure.ok()) first_failure = s;
: } else if (s.ok()) {
: blocks.emplace_back(block_id, lb);
: } else {
: deleted->emplace_back(block_id);
: }
: }
: }
Having one class grab the internal lock of another class is a code smell and should be avoided unless absolutely necessary. In this case, surely this could be a private LogBlockManager function?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435
PS5, Line 1435: return container_->block_manager()->DeleteBlock(id());
Hmm, I'd really like to eliminate DeleteBlock() altogether, as that would simplify your current patch. Remind me why we didn't remove DoCloseBlocks() and DeleteBlock() from here?
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1897
PS5, Line 1897: container
Nit: it's not a container in this code path.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2604
PS5, Line 2604: // holes belong to the same container can be coalesced.
belonging
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2610
PS5, Line 2610: b->RegisterDeletion(shared_transaction);
: transaction->AddBlock(b);
It seems like these two are always called together. Perhaps they can be combined into one function?
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 23:45:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h@299
PS2, Line 299: Blocks belonging to read-only containers will not be deleted.
> 'Blocks belonging to read-only containers will not be deleted.
Done
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1167
PS2, Line 1167: one hole punc
> s/hole punching/one hole punch
Done
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1174
PS2, Line 1174: void AddBlock(const scoped_refptr<internal::LogBlock>& lb);
> I think this would be more clear if it were named 'AddBlock'. 'Update' is
Done
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1196
PS2, Line 1196: "
> s/at/in
Done
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1202
PS2, Line 1202:
> * with the type
Done
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1207
PS2, Line 1207:
> Hoist this check/increment out of the loop and use IncrementBy
Done
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1220
PS2, Line 1220: Status LogBlockDeletionTransaction::CommitDeletedBlocks(
> Since there are separate implementations of the transaction types now, perh
Yes, makes sense. Will do.
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1283
PS2, Line 1283: DCHECK(transaction);
> Can this be simplified to
I think only when multiple block deletions register to the same 'transaction'(which is L2556 and L1221), we will need ptr() here. Since L1221 already uses shared_from_this(). Maybe we can simplify it to transaction_ = transaction; if in L2556 also do the same.
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@2027
PS2, Line 2027: Status LogBlockManager::RemoveLogBlock(
> This comment should be moved down to the read_only() check
Done
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:14:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#5).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 286 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/5
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 10:
The failed test hash_util-test.cc seems to be a setup issue and should not be relevant to the change.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:57:33 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#4).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 297 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/4
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 10: Code-Review+2
I wonder if Dan wants to take another look, since this changed a bunch since he last looked at it.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:24:26 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM
......................................................................
Patch Set 2:
The failed test seems to be a known flaky test filed in https://issues.apache.org/jira/browse/KUDU-1736. Taking a look.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 28 Sep 2017 23:40:47 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 4:
Checking the failed tests. I should post the fix soon.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 00:22:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 10:
> I wonder if Dan wants to take another look, since this changed a
> bunch since he last looked at it.
Sure, would like to hear Dan's suggestions as well.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:58:55 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h@299
PS2, Line 299: The deletion is withdrawn for blocks belong to read-only container.
'Blocks belonging to read-only containers will not be deleted.
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1167
PS2, Line 1167: hole punching
s/hole punching/one hole punch
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1174
PS2, Line 1174: void UpdateDeletedBlockMap(const scoped_refptr<internal::LogBlock>& lb);
I think this would be more clear if it were named 'AddBlock'. 'Update' is ambiguous, it could be an add or remove.
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1196
PS2, Line 1196: at
s/at/in
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1202
PS2, Line 1202:
* with the type
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1207
PS2, Line 1207: container->block_manager()->metrics()->holes_punched->Increment();
Hoist this check/increment out of the loop and use IncrementBy
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1220
PS2, Line 1220: LogBlockManager* lbm = down_cast<LogBlockManager*>(bm_);
Since there are separate implementations of the transaction types now, perhaps it makes sense to store bm_ as the more specific type instead of down casting?
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1283
PS2, Line 1283: transaction_ = transaction->ptr();
Can this be simplified to
transaction_ = transaction;
At which point you don't need ptr()?
http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@2027
PS2, Line 2027: // Deletion is forbidden for read-only container.
This comment should be moved down to the read_only() check
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:52:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#11).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
9 files changed, 436 insertions(+), 189 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/11
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 7:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@284
PS6, Line 284: NO_FATALS(CheckLogMetrics(entity,
> Since you're already updating all of this, please switch to the shorter NO_
Done
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@382
PS6, Line 382: {1, &METRIC_block_manager_total_blocks_deleted} }));
> This might be flaky; the hole punch can happen as soon as deletion_transact
Hmm, misunderstood your previous suggestions. Updated.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@427
PS6, Line 427: {11, &METRIC_block_manager_total_blocks_deleted} }));
> This could be flaky, see above.
Done
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435
PS5, Line 1435: DCHECK(state_ == CLEAN || state_ == DIRTY)
> My working theory is that much of the complexity of this patch is because w
I see, I remember the reasons we leave DoCloseBlocks() and DeleteBlock() instead of do noting here are: 1) Abort() can happen in a normal phrase of tserver and current hole repunching cannot clean these gaps. 2) we are going to implement a fine grained abort handling as documented above in future.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1151
PS6, Line 1151: }
> Only if PunchHole actually succeeded though, right?
Right, updated.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1249
PS6, Line 1249: container->ToString()));
> I think CHECK_OK() is more appropriate if failure means some sort of bug on
Done
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1269
PS6, Line 1269: if (!s.ok()) {
> Can this be taken care of by RemoveLogBlocks() too?
Right, RegisterDeletion/AddBlock need to be made available for Repair(). And I separate RecordDeletion out from RemoveLogBlocks for the reason I mentioned below. Let me know if it makes sense to you.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2063
PS6, Line 2063: } else {
> std::move(lb) here.
Done
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2079
PS6, Line 2079: if (container->read_only()) {
> Make this a cref rather than a copy so you don't need to ref the LogBlock.
Done
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2112
PS6, Line 2112: BlockId block_id = lb->block_id();
> Any particular reason this isn't part of RemoveLogBlocks? Seems like there'
I think the main reason is I want to hold the LBM's lock as short as possible. Maybe that is not a big concern? If so, I can move it into RemoveLogBlocks.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2594
PS6, Line 2594: b->RegisterDeletion(transaction);
: transaction->AddBlock(b);
> What's this for? 'transaction' is already behind a shared_ptr.
I read here that you need to call shard_from_this() to return a new std::shared_ptr<T> that shares ownership (http://en.cppreference.com/w/cpp/memory/enable_shared_from_this). But after thinking more, this line is redundant, shared_ptr's copy assignment is good enough. Will remove.
Not calling NewDeletionTransaction here because it needs a cast from BlockDeletionTransaction to LogBlockDeletionTransaction later when call RegisterDeletion.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/util/metrics.h@384
PS6, Line 384: kHoles,
> See the comment on L361; you need to update Name() too.
Done
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:15:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8162
to look at the new patch set (#9).
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
KUDU-2055 [part 5]: Coalesce hole punch for LBM
This patch extends the implementation of BlockDeletionTransaction to
actually coalesce hole punch in LBM, so that blocks in one container
that are contiguous are grouped together in a hole punch operation.
It also adds a new metric 'holes_punched' in log block manager to track
the number of hole punching operations. And another two metrics
'blocks_created' and 'blocks_deleted' in block manager to track blocks
that were created and deleted since service start respectively.
It updates unit test LogBlockManagerTest.MetricsTest, to verify that
coalescing hole punching works as expected by checking the value of
'holes_punched' metric.
Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
9 files changed, 424 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8162/9
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 6:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@284
PS6, Line 284: ASSERT_NO_FATAL_FAILURE(CheckLogMetrics(entity,
Since you're already updating all of this, please switch to the shorter NO_FATALS macro.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@382
PS6, Line 382: { {0, &METRIC_log_block_manager_holes_punched},
This might be flaky; the hole punch can happen as soon as deletion_transaction goes out of scope.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@427
PS6, Line 427: { {1, &METRIC_log_block_manager_holes_punched},
This could be flaky, see above.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435
PS5, Line 1435: Status LogWritableBlock::AppendV(ArrayView<const Slice> data) {
> Sorry, I do not quite follow why do we need to remove DoCloseBlocks() here
My working theory is that much of the complexity of this patch is because we have to support two "delete block" code paths: one that's transaction-based, and one that isn't. AFAICT, this is the last place that uses DeleteBlock(), and it calls it (and DoCloseBlocks()) in order to fully delete a block on abort.
I remembered that Dan, you, and I discussed this behavior in the past and I was wondering if you remembered why we didn't eliminate the DoCloseBlocks() and DeleteBlock() calls back then.
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2610
PS5, Line 2610: internal::LogBlockContainer* container = FindPtrOrNull(containers_by_name,
:
> Yeah, previously I combined them. But you pointed out that there are circle
I meant have one function that performs both operations. But, such a function would need friendship into either LogBlockDeletionTransaction or LogBlock, so perhaps it's not worth it.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1151
PS6, Line 1151: metrics_->holes_punched->Increment();
Only if PunchHole actually succeeded though, right?
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1249
PS6, Line 1249: WARN_NOT_OK(CoalesceIntervals<int64_t>(&entry.second),
I think CHECK_OK() is more appropriate if failure means some sort of bug on our part.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1269
PS6, Line 1269: Status s = lbm_->RecordDeletion(lb);
Can this be taken care of by RemoveLogBlocks() too?
Another way to look at this: as compared to "part 4", this patch should change CommitDeletedBlocks() to call a new DeleteBlocks() instead of the old DeleteBlock(). I can't see a reason why all of this work can't be done by a single DeleteBlocks() method, possibly decomposed into private LBM methods if it helps readability.
The only catch is that RegisterDeletion/AddBlock need to be made available for Repair(), which is a little different as it doesn't delete from in-memory maps or from on-disk state.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2063
PS6, Line 2063: log_blocks->emplace_back(lb);
std::move(lb) here.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2079
PS6, Line 2079: scoped_refptr<internal::LogBlock>
Make this a cref rather than a copy so you don't need to ref the LogBlock.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2112
PS6, Line 2112: Status LogBlockManager::RecordDeletion(const scoped_refptr<LogBlock>& lb) {
Any particular reason this isn't part of RemoveLogBlocks? Seems like there'd be
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2594
PS6, Line 2594: shared_ptr<LogBlockDeletionTransaction> shared_transaction =
: transaction->shared_from_this();
What's this for? 'transaction' is already behind a shared_ptr.
Also, why doesn't this code path call NewDeletionTransaction to make the transaction?
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/util/metrics.h@384
PS6, Line 384: kHoles,
See the comment on L361; you need to update Name() too.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 16 Oct 2017 23:31:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 25 Oct 2017 18:46:53 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM
......................................................................
Patch Set 3:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@13
PS3, Line 13: It also adds a new metric 'holes_punched' in log block manager to track
Let's also add a new counter for 'blocks_deleted', which will track the total number of deletions since startup. By comparing it with holes_punched, we can see how effective the coalescing behavior is.
Actually, let's add 'blocks_deleted' and 'blocks_created' as generic block manager metrics (see block_manager_metrics.{cc,h}), since they're not specific to the LBM.
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@14
PS3, Line 14: operation
operations
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17
PS3, Line 17: vaule
value
http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17
PS3, Line 17: coalescing hole punch works as expected by checking the vaule of
punching
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h@323
PS3, Line 323: class BlockDeletionTransaction {
In part 3 I left a comment about how it'd be nice for these transaction classes to be purely virtual. If you agree, I think this bit of refactoring should be moved to part 3.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc@347
PS3, Line 347: std::shared_ptr<BlockDeletionTransaction> deletion_transaction =
Add a using:: for this.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@128
PS3, Line 128: METRIC_DEFINE_gauge_uint64(server, log_block_manager_holes_punched,
Because the number of holes punched only ever increases, this should be a counter not a gauge.
See "Gauge vs. Counter" in metrics.h for more information.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@129
PS3, Line 129: "Blocks Under Management",
Update.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@130
PS3, Line 130: kudu::MetricUnit::kBlocks,
Update.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@131
PS3, Line 131: "Number of data blocks currently under management");
Update.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@182
PS3, Line 182: scoped_refptr<AtomicGauge<uint64_t>> holes_punched;
Please separate from the previous two metrics with a blank line, since it's not an "under management" metric.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@229
PS3, Line 229: to
with
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@230
PS3, Line 230: gets
is
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@246
PS3, Line 246: that this block has been registered to
with which this block has been registered.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208
PS3, Line 1208: if (container->block_manager()->metrics()) {
: container->block_manager()->metrics()->holes_punched->IncrementBy(
: entry.second.size());
: }
For this to be more accurate:
1. Defer it to ContainerDeletionAsync, so that the change in metric values reflect the time that it actually takes to process the hole punches.
2. Only do it if hole punching actually succeeds.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1224
PS3, Line 1224: Status s = lbm_->RemoveLogBlock(block, shared_from_this());
Would be better if:
1. shared_from_this() were called once; no need to call it for every iteration in the loop.
2. RemoveLogBlock() was actually RemoveLogBlocks() to avoid a spinlock acquisition/release with each loop iteration.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1286
PS3, Line 1286: transaction_->AddBlock(this);
The call sequence for deleting blocks is long and involves a lot of class traversal:
1. LogBlockManager::DeletionTransaction() to get a new transaction. Also calls BlockDeletionTransaction and LogBlockDeletionTransaction constructors.
2. BlockDeletionTransaction::AddDeletedBlock to mark a block as to-be-deleted.
3. LogBlockDeletionTransaction::CommitDeletedBlocks to effect the deletions in-memory and to set them up to be deleted on disk. This calls LogBlockManager::RemoveLogBlock, which calls LogBlock::RegisterDeletion, which calls LogBlockDeletionTransaction::AddBlock. Quite the back-and-forth.
4. ~LogBlockDeletionTransaction to effect the on-disk deletions.
It's step 3 in particular that I find difficult to grok. Here are some ideas for simplification:
- Make BlockCreationTransaction and BlockDeletionTransaction pure virtual. That eliminates at least one hop, since these interfaces won't have any code in them.
- Change deletion transaction semantics to be two-step instead of three-step. Right now the three distinct steps are: 1) AddDeletedBlock to show that you intend to delete a block. 2) CommitDeletedBlocks on the entire transaction to do the in-memory deletions. 3) When the transaction goes out of scope, the on-disk data is deleted. Perhaps step #2 could be done as part of step #3?
- Consider adding another patch before this one to force all deletions through BlockDeletionTransaction, thus allowing you to eliminate BlockManager::DeleteBlock. By doing so, you might be able to refactor the LBM more thoroughly because various internal deletion-related functions won't need to support multiple callers.
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@2550
PS3, Line 2550: // TODO(adar): can be more efficient (less fs work and more space reclamation
: // in case of misaligned blocks) via hole coalescing first, but this is easy.
Remove/update.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:26:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435
PS5, Line 1435: DCHECK(state_ == CLEAN || state_ == DIRTY)
> I see, I remember the reasons we leave DoCloseBlocks() and DeleteBlock() in
Okay, point #1 is the important one.
Anyway, using a one-block deletion transaction here seems fine for now.
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2112
PS6, Line 2112: BlockId block_id = lb->block_id();
> I think the main reason is I want to hold the LBM's lock as short as possib
This logic can be in RemoveLogBlocks but called outside the spinlock. If RemoveLogBlocks is too large and unwieldy, at least this can be a private helper called by RemoveLogBlocks (that is, no reason why anyone but RemoveLogBlocks should be aware of it).
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:58:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 9:
(5 comments)
Looks much better!
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.h@296
PS9, Line 296: // blocks were already deleted, e.g encounter 'NotFound' error during removal.
encountered
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@1247
PS9, Line 1247: CHECK_OK_PREPEND(CoalesceIntervals<int64_t>(&entry.second),
Didn't even know we had CHECK_OK_PREPEND. Neat!
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@2064
PS9, Line 2064: for (auto& lb : lbs) {
Can this be const auto& ?
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@2075
PS9, Line 2075: .CloneAndPrepend(
: "Unable to append deletion record to block metadata");
Only do CloneAndPrepend() if !s.ok().
http://gerrit.cloudera.org:8080/#/c/8162/9/src/kudu/fs/log_block_manager.cc@2126
PS9, Line 2126: mem_tracker_->Release(kudu_malloc_usable_size(lb->get()));
:
: if (metrics()) {
: metrics()->blocks_under_management->Decrement();
: metrics()->bytes_under_management->DecrementBy((*lb)->length());
: }
Now that we're passing 'lb' back to RemoveLogBlocks, we could consolidate all of these across LogBlocks into just three calls.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 18 Oct 2017 21:47:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 10:
(5 comments)
Looks good to me, my only real feedback is to add a comment to the LBM deletion transaction dtor, as I verbosely explained in the comment.
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@260
PS10, Line 260: FindOrNull
I think it'd be better to do a FindOrDie here to be more explicit about what happens when the metric isn't found. Dereferencing null should be equivalent, but I'd expect the error message from FindOrDie to be easier to debug.
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@266
PS10, Line 266: entity->FindOrNull(*prototype).get())->value());
likewise
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager-test.cc@1069
PS10, Line 1069: {
Are these explicit blocks necessary for correctness, or is it just to add a little bit of scoping?
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc@231
PS10, Line 231: destructed
should this be committed?
http://gerrit.cloudera.org:8080/#/c/8162/10/src/kudu/fs/log_block_manager.cc@1244
PS10, Line 1244: LogBlockDeletionTransaction::~LogBlockDeletionTransaction() {
What is the reason to do this work in the destructor instead of in CommitDeletedBlocks? I'm a bit surprised we're doing work in the destructor, since the FBM equivalent doesn't do anything in the destructor, and nothing in the BlockDeletionTransaction says anything about the dtor. I asked in the test file about the braces being added, and now I see this is probably the reason. Could we do all of this in CommitDeletedBlocks instead? Perhaps the dtor could be empty except for a DCHECK that the transaction has previously been committed.
Edit: OK I think I've figured this out. We're using the LogBlockDeletionTransaction's shared ownership to keep it alive until the very last block is destructed, at which point the actual hole punching takes place. I think this is sufficiently tricky that you should document this lifetime semantics on this dtor method.
It also changes the semantics so that no hole punching is attempted until the last block of the transaction is dropped, but I think that's probably fine, I can't see a reason why it would cause problems.
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:53:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 5]: Coalesce hole punch for LBM
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208
PS3, Line 1208:
: virtual Status CommitDeletedBlocks(std::vector<BlockId>* deleted) override;
:
: //
> The hole punches are queued in the data directory's thread pool, so if you
I see, thanks!
--
To view, visit http://gerrit.cloudera.org:8080/8162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275
Gerrit-Change-Number: 8162
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 07:00:31 +0000
Gerrit-HasComments: Yes