Hao Hao has posted comments on this change.

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


Patch Set 14:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/7207/13//COMMIT_MSG
Commit Message:

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


PS13, Line 15:  
> no comma here
Done


PS13, Line 15: reused
> reused
Done


Line 19: 
https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing
> I think you may need to make this viewable from anyone (I tried in an incog
Done


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

PS13, Line 74: resulti
> s/results/resulting
Done


Line 88:     // No more data may be written to the block, but it is not yet 
guaranteed
> 'not yet closed' doesn't really distinguish this state from CLOSED.  I thin
Done


Line 281: // Group a set of block writes together in a transaction. This has two
> prefer 'BlockTransaction() = default;'
Done


Line 283: // synchronization for a batch of blocks if possible to achieve better
> Can't this just be set to the default dtor?
Done


PS13, Line 288: 
> emplace_back
Done


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


Line 305:     Status s = bm->CloseBlocks(created_blocks_);
> Would it be difficult to change BlockManager::CloseBlocks to take a 'const 
Updated it as you suggested.


http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:

Line 727: 
> I think this should be doing an async flush regardless of the block_manager
Done


http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/log_block_manager.cc
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 sta
Sorry, I do not quite follow your comment. You mean FinishBlock is always 
called with 's' as OK status? It does not seem to be the case, either Sync() or 
AppendMetadata() can return bad status, right?


PS13, Line 522: unnecessary I
> unnecessary
Done


Line 1234:   // Actually close the block, possibly synchronizing its dirty data 
and
> Could DoClose and CloseFlags be private?
Hmm, they are used in other class such as LogBlockManager.


Line 1911:   record.set_timestamp_us(GetCurrentTimeMicros());
> Same thing here - I think we want to do this depending only on the block_co
Done


PS13, Line 1917:  that 
> s/belong/belonging
Done


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