[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM

[Adar Dembo (Code Review)]

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 
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 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 

[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM

[Hao Hao (Code Review)]

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& 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:14:28 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM

[Hao Hao (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM

[Dan Burkert (Code Review)]

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& 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(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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:52:21 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM

[Hao Hao (Code Review)]

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 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 28 Sep 2017 23:40:47 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2055 [part 4]: Coalesce hole punch for LBM

[Hao Hao (Code Review)]

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