[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7656 )

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager-test.cc@1109
PS9, Line 1109: forcing
force


http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h@342
PS9, Line 342:   deleted_blocks_.clear();
Shouldn't we clear regardless of error?


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@321
PS8, Line 321: the list of block
 :   // IDs that were successfully deleted.
> I put it as 'can be' because at this point, the blocks may not be deleted y
Yeah, I think the asynchronous deletion is a bit of an implementation detail. 
For example, the deletion will happen staight-away in the FBM; the fact that 
the on-disk file may remain around until the last opener disappears is a UNIX 
filesystem detail.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-Change-Number: 7656
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 21 Sep 2017 23:31:00 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7656 )

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 9:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7656/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/7656/9//COMMIT_MSG@7
PS9, Line 7: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of
I think the message needs to be on a single line for most systems (e.g. GitHub) 
to handle it correctly.  It's better to be a bit over the limit than wrapped in 
this case


http://gerrit.cloudera.org:8080/#/c/7656/9//COMMIT_MSG@22
PS9, Line 22: abstraction 'BlockDeletionTransaction', and doesn't yet change any
In that case it may be better to change the title to something like 'Add 
fs::BlockDeletionTransaction API', and save the current title for the commit 
which does coalescing.


http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h@325
PS9, Line 325:   Status CommitDeletedBlocks(std::vector* deleted) {
The 'deleted' out parameter is a little bit unusual here, in that it's only 
used in the case of an error.  Is having the list of deleted blocks in the 
error case actionable (is it being used in a follow-up commit)?  Would it be 
sufficient just to set the number of deleted blocks?


http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h@333
PS9, Line 333: deleted->emplace_back(block);
Should the block be added even if it's status is NotFound?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-Change-Number: 7656
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 21 Sep 2017 20:37:42 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7656 )

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 9:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager-test.cc@1102
PS8, Line 1102:   
ASSERT_OK(deletion_transaction.CommitDeletedBlocks(_blocks));
> ASSERT_OK on this?
Done


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@324
PS8, Line 324: that was seen, if any.
> that was seen, if any
Done


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@326
PS8, Line 326: Status first_failure;
> How about calling this "first_failure"?
Done


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@329
PS8, Line 329: then the block was already deleted.
 :   if (!s.ok()
> then the block was already deleted
Done


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@339
PS8, Line 339:
> We typically combine statuses by prepending, not appending.
Done


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@340
PS8, Line 340:
> Don't terminate status messages with periods since they're often combined w
Done


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@339
PS8, Line 339:"first failure",
 :   
> Should be something like "only deleted $0 blocks, first failure"
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-Change-Number: 7656
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 21 Sep 2017 18:45:04 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..

KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of
blocks

Similar to 'BlockCreationTransaction', this patch adds a new layer of
abstraction at Block Manager to coalesce blocks deletions in a logical
operation, e.g. compaction.

By coalescing blocks deletions, the number of holes punched in LBM will
be reduced from one per block to one per log block container in the
best case (that the individual holes in one container are all
contiguous). This should overall optimize the performance of operation
that involves batch deletions, such as tablet deletion and hole
repunching.

This patch is the first part of a series. It only adds the new
abstraction 'BlockDeletionTransaction', and doesn't yet change any
deletion semantics or improve performance.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client.cc
11 files changed, 109 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/9
--
To view, visit http://gerrit.cloudera.org:8080/7656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-Change-Number: 7656
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7656 )

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@321
PS8, Line 321: the list of IDs of
 :   // block that can be successfully deleted.
> the list of block IDs that were successfully deleted.
I put it as 'can be' because at this point, the blocks may not be deleted yet. 
But if you think this is more accurate, I can update it based on your 
suggestion.


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@331
PS8, Line 331:   if (!s.ok() && !s.IsNotFound() && !status.ok()) {
> This should be status.ok(), right? Not !status.ok()?
Right, my bad...


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h@334
PS8, Line 334: deleted->emplace_back(block);
> Seems like you could end up here if !status.ok() but e.g. s.IsIOError().
Will update.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-Change-Number: 7656
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 21 Sep 2017 04:59:46 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 8:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

Line 1102:   Status s = 
deletion_transaction.CommitDeletedBlocks(_blocks);
ASSERT_OK on this?


http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

PS8, Line 321: the list of IDs of
 :   // block that can be successfully deleted.
the list of block IDs that were successfully deleted.


PS8, Line 324: that has been seen, if there is any
that was seen, if any


Line 326: Status status;
How about calling this "first_failure"?


PS8, Line 329: then the block could be already successfully
 :   // deleted.
then the block was already deleted


Line 331:   if (!s.ok() && !s.IsNotFound() && !status.ok()) {
This should be status.ok(), right? Not !status.ok()?


Line 334: deleted->emplace_back(block);
Seems like you could end up here if !status.ok() but e.g. s.IsIOError().


PS8, Line 339: CloneAndAppend
We typically combine statuses by prepending, not appending.


PS8, Line 340: .
Don't terminate status messages with periods since they're often combined with 
other statuses, at which point they're separated with colons.


PS8, Line 339: the number of blocks could "
 : "not be deleted
Should be something like "only deleted $0 blocks, first failure"

Though you should produce this in a test and see what it looks like.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7656/7//COMMIT_MSG
Commit Message:

PS7, Line 14: hole
> holes
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..

KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of
blocks

Similar to 'BlockCreationTransaction', this patch adds a new layer of
abstraction at Block Manager to coalesce blocks deletions in a logical
operation, e.g. compaction.

By coalescing blocks deletions, the number of holes punched in LBM will
be reduced from one per block to one per log block container in the
best case (that the individual holes in one container are all
contiguous). This should overall optimize the performance of operation
that involves batch deletions, such as tablet deletion and hole
repunching.

This patch is the first part of a series. It only adds the new
abstraction 'BlockDeletionTransaction', and doesn't yet change any
deletion semantics or improve performance.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client.cc
11 files changed, 91 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7656/7/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 284: BlockManager* bm = created_blocks_[0]->block_manager();
> Since BlockDeletionTransaction can't infer the BlockManager from the delete
Done


Line 309:   void AddDeletedBlock(const BlockId& block) {
> BlockIds are small (64-bits each) so it's preferred to pass them by value a
Done


PS7, Line 317:   // Returns the list of IDs, among the given blocks IDs, of 
block that can be
 :   // successfully deleted.
> Why doesn't this have the same signature as CommitCreatedBlocks?
Done


Line 324: WARN_NOT_OK(s, strings::Substitute("Could not delete block 
$0", block.ToString()));
> Seems like an unusual place for a warning. I'd suggest an alternative:
Sure, but I would like to add a output parameter to include which blocks are 
successfully deleted. In case like TabletMetadata::DeleteOrphanedBlocks, may 
need information as such.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 7:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7656/7//COMMIT_MSG
Commit Message:

PS7, Line 14: hole
holes


http://gerrit.cloudera.org:8080/#/c/7656/7/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 284: BlockManager* bm = created_blocks_[0]->block_manager();
Since BlockDeletionTransaction can't infer the BlockManager from the deleted 
blocks themselves, let's make this consistent with it by supplying the 
BlockManager in the transaction's constructor.


Line 309:   void AddDeletedBlock(const BlockId& block) {
BlockIds are small (64-bits each) so it's preferred to pass them by value and 
copy them.


PS7, Line 317:   // Returns the list of IDs, among the given blocks IDs, of 
block that can be
 :   // successfully deleted.
Why doesn't this have the same signature as CommitCreatedBlocks?


Line 324: WARN_NOT_OK(s, strings::Substitute("Could not delete block 
$0", block.ToString()));
Seems like an unusual place for a warning. I'd suggest an alternative:

1. Return a Status.
2. That Status should be the first failure seen.
3. Add to the Status a message including the total number of blocks that could 
NOT be deleted.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 170: class BlockDeletionTransaction : public 
RefCountedThreadSafe {
> Yes, but the main reason for doing so is to improve performance by coalesci
OK, do it in a separate patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 170: // used to specify directories based on block type (e.g. to prefer 
bloom block
> Won't you need some sort of shared ownership in order to allow BlockDeletio
Yes, but the main reason for doing so is to improve performance by coalescing 
hole punching belongs to the same container (Otherwise, we do not need to make 
it a shared ownership). Given we do not want to 'change any deletion semantics 
or improve performance' in this patch, I feel we can move this logic into next 
patch. But I am open to either options (either put it in this patch or next 
one), if you have a stronger preference.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 170: class BlockDeletionTransaction : public 
RefCountedThreadSafe {
> I see, thanks! Would you mind I make it shard_ptr in next patch. After thin
Won't you need some sort of shared ownership in order to allow 
BlockDeletionTransaction to be incorporated into LogBlockManager::DeleteBlock 
though?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..

KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of
blocks

Similar to 'BlockCreationTransaction', this patch adds a new layer of
abstraction at Block Manager to coalesce blocks deletions in a logical
operation, e.g. compaction.

By coalescing blocks deletions, the number of hole punched in LBM will
be reduced from one per block to one per log block container in the
best case (that the individual holes in one container are all
contiguous). This should overall optimize the performance of operation
that involves batch deletions, such as tablet deletion and hole
repunching.

This patch is the first part of a series. It only adds the new
abstraction 'BlockDeletionTransaction', and doesn't yet change any
deletion semantics or improve performance.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
2 files changed, 53 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7656/6//COMMIT_MSG
Commit Message:

PS6, Line 14: hole punching
> holes punched
Done


Line 15: will be reduced from one per block to one per log block container.
> That's assuming that the individual holes are all contiguous. In the worst 
Right. Thanks for catching it.


PS6, Line 17: compaction
> I actually think tablet deletion is a better example, since that'll delete 
Done


PS6, Line 19: serial
> series
Done


http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 170: class BlockDeletionTransaction : public 
RefCountedThreadSafe {
> Yeah, I think Dan mentioned this to me the other day.
I see, thanks! Would you mind I make it shard_ptr in next patch. After thinking 
more I would like to narrow down the scope of this patch just to introduce 
BlockDeletionTransaction.


Line 180:   virtual std::vector MarkDeletions(std::vector 
blocks) = 0;
> I think this should be structured like BlockCreationTransaction in that:
Done


PS6, Line 259:   // Creates a new block deletion transaction block, which can 
be used
 :   // to coalesce a group of block deletions.
 :   virtual scoped_refptr 
CreateDeletionTransaction() = 0;
> Can we start with BlockDeletionTransaction behaving more like BlockCreation
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 170: class BlockDeletionTransaction : public 
RefCountedThreadSafe {
> Sure, just curious I saw in our contribution page (https://kudu.apache.org/
Yeah, I think Dan mentioned this to me the other day.

The big minus against shared_ptr (two allocations) is no longer true with C++11 
and std::make_shared. Given that shared_ptr is usable with weak_ptr (and thus 
allows for more flexible object lifetimes, especially with lambdas), I think 
that's enough to favor it over scoped_refptr in the general case.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

PS6, Line 165: deletion
> deletions
Done


Line 170: class BlockDeletionTransaction : public 
RefCountedThreadSafe {
> Would prefer if this used std::shared_ptr rather than scoped_refptr. We gen
Sure, just curious I saw in our contribution page 
(https://kudu.apache.org/docs/contributing.html#_pointers), saying that 'Since 
scoped_refptr is generally faster and smaller, try to use it rather than 
shared_ptr in new code.' Is this statement outdated? Not sure if I missed 
something?


Line 288: class BlockCreationTransaction {
> Could you do this rename as part of a separate patch? It's clearly a trivia
Thanks for the suggestion! Done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 6:

(9 comments)

Here's my first pass. I'll do a deeper pass after you've addressed these 
comments.

http://gerrit.cloudera.org:8080/#/c/7656/6//COMMIT_MSG
Commit Message:

PS6, Line 14: hole punching
holes punched


Line 15: will be reduced from one per block to one per log block container.
That's assuming that the individual holes are all contiguous. In the worst 
case, there's still one hole punched per block, right?


PS6, Line 17: compaction
I actually think tablet deletion is a better example, since that'll delete a 
great many blocks which were often written contiguously.


PS6, Line 19: serial
series


http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

PS6, Line 165: deletion
deletions


Line 170: class BlockDeletionTransaction : public 
RefCountedThreadSafe {
Would prefer if this used std::shared_ptr rather than scoped_refptr. We 
generally prefer std::shared_ptr for new code, unless there's a good reason not 
to.

Also, can you move this down to just after BlockCreationTransaction?


Line 180:   virtual std::vector MarkDeletions(std::vector 
blocks) = 0;
I think this should be structured like BlockCreationTransaction in that:
1. Deletions are "added" to the ongoing tracking apparatus.
2. The transaction is "committed" when it is ready.


PS6, Line 259:   // Creates a new block deletion transaction block, which can 
be used
 :   // to coalesce a group of block deletions.
 :   virtual scoped_refptr 
CreateDeletionTransaction() = 0;
Can we start with BlockDeletionTransaction behaving more like 
BlockCreationTransaction? That is, created separately?

Later on we can change the semantics of both transaction classes such that 
they're created from a block manager.


Line 288: class BlockCreationTransaction {
Could you do this rename as part of a separate patch? It's clearly a trivial 
patch and would make it easier to review the new BlockDeletionTransaction.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups 
of blocks
..

KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of
blocks

Similar to 'BlockCreationTransaction', this patch adds a new layer
of abstraction at Block Manager to coalesce blocks deletions in a
logical operation, e.g. compaction.

By coalescing blocks deletions, the number of hole punching in LBM
will be reduced from one per block to one per log block container.
This should overall optimize the performance of operation that
involves batch deletions, such as compaction and hole repunching.

This patch is the first part of a serial. It only adds the new
abstraction 'BlockDeletionTransaction', and doesn't yet change
any deletion semantics or improve performance.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.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
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tserver/tablet_copy_client.h
19 files changed, 157 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-2055[part 1]: Coalesce hole punching when deleting groups 
of blocks
..


Patch Set 4:

The failed test TestCopyFromCrashedSource is a known flaky test (KUDU-2109).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2055[part 1]: Coalesce hole punching when deleting groups 
of blocks
..

KUDU-2055[part 1]: Coalesce hole punching when deleting groups of
blocks

Similar to 'BlockCreationTransaction', this patch adds a new layer
of abstraction at Block Manager to coalesce blocks deletions in a
logical operation, e.g. compaction.

By coalescing blocks deletions, the number of hole punching in LBM
will be reduced from one per block to one per log block container.
This should overall optimize the performance of operation that
involves batch deletions, such as compaction and hole repunching.

This patch is the first part of a serial. It only adds the new
abstraction 'BlockDeletionTransaction', and doesn't yet change
any deletion semantics or improve performance.

Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.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
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tserver/tablet_copy_client.h
19 files changed, 225 insertions(+), 95 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot