Dan Burkert has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager

Patch Set 13:


Commit Message:

PS5, Line 12: potentionaly
typo: potentially

Commit Message:

PS13, Line 10: creates
s/creates which happen/new blocks

PS13, Line 14: I FinalizeWrite() t
add commas (API, FinalizeWrite(), to...)

PS13, Line 15: resued

PS13, Line 15: ,
no comma here

Line 19: 
I think you may need to make this viewable from anyone (I tried in an incognito 
window and it asked me to log into my cloudera gmail account).

File src/kudu/fs/block_manager.h:

PS13, Line 74: results

Line 88:     // No more data may be written to the block, but it is not yet 
'not yet closed' doesn't really distinguish this state from CLOSED.  I think 
this would be more clear as 'but it is not yet guaranteed to be durably stored 
on disk'.

Line 281:   BlockTransaction() {}
prefer 'BlockTransaction() = default;'

Line 283:   ~BlockTransaction() {
Can't this just be set to the default dtor?

   ~BlockTransaction() = default;

PS13, Line 288: push_back(

PS13, Line 293: CommitCreation
Might be more clear as 'CommitCreatedBlocks'.

Line 305:     Status s = bm->CloseBlocks(blocks);
Would it be difficult to change BlockManager::CloseBlocks to take a 'const 
vector<unique_ptr<WritableBlock>>&' so you don't have to make a vector copy 
here?  If that would require changing a bunch of other call sites, feel free to 

File src/kudu/fs/file_block_manager.cc:

Line 727:     // dirty pages if 'block_manager_flush_on_finalize' is true.
I think this should be doing an async flush regardless of the 
block_manager_flush_on_finalize flag value.  Unfortunately I think that means 
you will need to copy most of the body of Finalize and remove the check (maybe 
abstract it out?), but I think we definitely want to parallel flushing to 
happen here.  block_coalesce_close shouldn't be tied to the flush on finalize 

File src/kudu/fs/log_block_manager.cc:

Line 278:   Status FinishBlock(const Status& s, WritableBlock* block,
It looks like the only place that calls FinishBlock calls it with an OK status, 
so I think the s param can be removed.

PS13, Line 522: unnecessarily

Line 1234:   Status DoClose(CloseFlags flags);
Could DoClose and CloseFlags be private?

Line 1911:     // dirty pages if 'block_manager_flush_on_finalize' is true.
Same thing here - I think we want to do this depending only on the 
block_coalesce_close flag.

PS13, Line 1917: belong

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a88884bd
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

Reply via email to