[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change.
Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager
..
Patch Set 8:
(41 comments)
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/bloomfile.h
File src/kudu/cfile/bloomfile.h:
PS8, Line 47: blocks
> It's just one block, right? Why 'blocks'?
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:
Line 283: block_->Finalize();
> RETURN_NOT_OK.
Done
Line 289: block_->Finalize();
> RETURN_NOT_OK.
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/cfile_writer.h
File src/kudu/cfile/cfile_writer.h:
PS8, Line 109: blocks
> Just one block.
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:
Line 396:
> Nit: why add an empty line here and not in the WriterThread/ReaderThread fu
Removed.
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
Line 995: ASSERT_OK(transaction.CommitWrites());
> Before committing, how about trying to open all the blocks in block_ids and
It looks like it is not returning not found error for FileBlockManager before
the block is closed. Not sure if it is that expected?
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
Line 143:
> I think FLAGS_cfile_do_on_finish has mostly outlived its usefulness (it was
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
PS8, Line 73: For LogWritableBlock, container
: //is released for further usage to avoid unnecessarily
creating new containers.
> Generalize this example so that it doesn't refer to implementation details.
Done
PS8, Line 76: For LogWritableBlock,
: //it also finishes the blocks belong to the same container
together to
: //reduce fsync() usage.
> Generalize this so that it doesn't refer to a particular implementation.
Done
Line 91: // The block is finalized as it has been fully written.
> How about: "There is some dirty data in the block and no more may be writte
Updated. But I think FINALIZED does not necessarily mean 'There is some dirty
data in the block'
Line 146: // Finalize a fully written block.
> This doesn't explain what restrictions the FINALIZED state places on the Wr
Done
Line 147: virtual Status Finalize() = 0;
> Nit: since this has a real effect on the state of the block, please move it
Done
Line 149: virtual int64_t offset() const { return 0; }
> I don't think this belongs in WritableBlock. It doesn't make any sense to a
Removed.
Line 285: // Group a set of blocks writes together in a transaction.
> The names and comments here conflate "write" and "create". Please choose on
Done
PS8, Line 289: BlockTransaction
> Besides the name, what's the difference between ScopedWritableBlockCloser a
Done.
PS8, Line 305: Status CommitWrites
> Can you add docs to this and AddCreatedBlock? Their names aren't as straigh
Done
Line 320: std::vector created_blocks_;
> Since we're now writing C++11 code, could you change this to be a vector of
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
Line 237: virtual Status Finalize() OVERRIDE;
> Nit: move to be just above BytesAppended().
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
Line 377: // Test a container can be reused when the block is finalized
> Please also add a variant of this for block_manager-test, to exercise FBM c
Done
Line 382: unique_ptr writer;
> The writer goes out of scope (and thus is closed) with every iteration of t
Done
Line 385: writer->Finalize();
> ASSERT_OK
Done
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
PS8, Line 270: have been finalized
> What happens if this is called and the blocks are not finalized?
Slightly updated the logic. This should only be called if a block is finished.
Line 272: Status FinishBlocks(std::vector blocks);
> Don't need std:: prefix.
Done
PS8, Line 365: 'should_update' indicates if it should round up
> I think we try to avoid boolean parameters with enums (e.g. a AlignmentAdju
Done
PS8, Line 810: ner file " << data_file_->filename();
> This is syncing the data file and metadata file, right? may need another VL
Done
PS8, Line 812: ata());
: RETURN_NOT_OK(SyncMet
> Why do we need this here and not in FinishBlock?
FinishBlock is called within DoClose() which is SyncMode is 'Sync'. That
indicates data/metadata is already been synced or will be synced later.
PS8, Line 1174: true
> maybe enum here too
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#9). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates which happen in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. A design doc can be found here: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/fs_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/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 19 files changed, 544 insertions(+), 242 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/9 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Andrew Wong has posted comments on this change. Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS8, Line 289: BlockTransaction Besides the name, what's the difference between ScopedWritableBlockCloser and BlockTransaction? Is there a different expected usage now? If so, maybe document it somewhere. PS8, Line 305: Status CommitWrites Can you add docs to this and AddCreatedBlock? Their names aren't as straightforward as AddBlock and CloseBlocks. Also would be worth noting any new behavior. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: PS3, Line 237: IDE; nit: use lower-case http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS8, Line 270: have been finalized What happens if this is called and the blocks are not finalized? PS8, Line 365: 'should_update' indicates if it should round up I think we try to avoid boolean parameters with enums (e.g. a AlignmentAdjustmentType or something with ROUND_UP and NO_ROUNDING). PS8, Line 810: ner file " << data_file_->filename(); This is syncing the data file and metadata file, right? may need another VLOG above the SyncMetadata() call PS8, Line 812: ata()); : RETURN_NOT_OK(SyncMet Why do we need this here and not in FinishBlock? PS8, Line 1174: true maybe enum here too? PS8, Line 1264: VLOG(3) << "Finalizing block " << id(); Should we be logging even if it's CLEAN? -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has posted comments on this change.
Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager
..
Patch Set 8:
(33 comments)
I reviewed everything outside the LBM, and then began reviewing the LBM
changes. I stopped because I saw two pretty serious issues that we need to
think about:
Container thread safety. WritableBlocks aren't supposed to be shared by
multiple threads, which meant that containers were also single threaded as
blocks were added to them. Now that's no longer the case, and this introduces
some synchronization issues when accessing container state. I think they can be
addressed with the introduction of a per-container lock (and careful thinking
about the right order in which to acquire the container lock and the LBM lock).
Consistent container in-memory accounting when operations fail. KUDU-1793 was a
serious issue, and to fix it, I tried to guarantee that container in-memory
state is only updated after the "point of no return" (that is, when a block was
guaranteed to be successfully written). This change explicitly does away with
that due to how Finalize() works. There's no way around this, but it means we
need to think very carefully about what happens to container in-memory state in
the event of a failure after a block is finalized.
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/bloomfile.h
File src/kudu/cfile/bloomfile.h:
PS8, Line 47: blocks
It's just one block, right? Why 'blocks'?
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:
Line 283: block_->Finalize();
RETURN_NOT_OK.
Line 289: block_->Finalize();
RETURN_NOT_OK.
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/cfile/cfile_writer.h
File src/kudu/cfile/cfile_writer.h:
PS8, Line 109: blocks
Just one block.
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:
Line 396:
Nit: why add an empty line here and not in the WriterThread/ReaderThread
functions, which do the same thing? Or was this a mistake?
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
Line 995: ASSERT_OK(transaction.CommitWrites());
Before committing, how about trying to open all the blocks in block_ids and
verifying that they can't be found?
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
Line 143:
> yes, there is some cases the block is flushed but not finalized, you can ca
I think FLAGS_cfile_do_on_finish has mostly outlived its usefulness (it was
used for performance experiments early on). So let's merge the "block is
finalized" and "block is flushing" states. This means you should make two
gflags changes:
- Change cfile_do_on_finish to something like cfile_close_block_on_finish. It'd
be a boolean with a default value of false.
- Add something like block_manager_flush_on_finalize. It'd be a boolean with a
default value of true.
http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
PS8, Line 73: For LogWritableBlock, container
: //is released for further usage to avoid unnecessarily
creating new containers.
Generalize this example so that it doesn't refer to implementation details.
PS8, Line 76: For LogWritableBlock,
: //it also finishes the blocks belong to the same container
together to
: //reduce fsync() usage.
Generalize this so that it doesn't refer to a particular implementation.
Line 91: // The block is finalized as it has been fully written.
How about: "There is some dirty data in the block and no more may be written."
Line 146: // Finalize a fully written block.
This doesn't explain what restrictions the FINALIZED state places on the
WritableBlock. Can I keep writing to it? Can I call FlushDataAsync() on it? Is
the block data now durably on disk? Use the comments in
FlushDataAsync()/Append()/Close() for inspiration.
Line 147: virtual Status Finalize() = 0;
Nit: since this has a real effect on the state of the block, please move it to
be just above BytesAppended(). That way the "simple accessor" functions are
grouped together, and the "complicated" functions likewise.
Line 149: virtual int64_t offset() const { return 0; }
I don't think this belongs in WritableBlock. It doesn't make any sense to
anyone outside a block manager implementation, because it's not even clear what
it's an offset into (a file? a container? something else?). Why do you need it?
Line 285: // Group a set of blocks writes together in a transaction.
The names and comments here conflate "write" and "create". Please choose one
and use it consistently.
Line 320: std::vector created_blocks_;
Since we're now writing C++11 code, could you chan
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#8). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates which happen in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. A design doc can be found here: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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-stress-test.cc 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/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 18 files changed, 344 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/8 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change.
Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager
..
Patch Set 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7207/5/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
Line 326:
> warning: the parameter 'block_ids' is copied for each invocation but only u
Done
http://gerrit.cloudera.org:8080/#/c/7207/6/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
Line 379: TEST_F(LogBlockManagerTest, TestFinalizeBlock) {
> error: unknown type name 'BlockManagerTest'; did you mean 'LogBlockManagerT
Done
Line 379: TEST_F(LogBlockManagerTest, TestFinalizeBlock) {
> error: use of undeclared identifier 'BlockManagerTest'; did you mean 'LogBl
Done
Line 379: TEST_F(LogBlockManagerTest, TestFinalizeBlock) {
> error: unknown class name 'BlockManagerTest'; did you mean 'LogBlockManager
Done
Line 388: ASSERT_EQ(1, bm_->all_containers_by_name_.size());
> error: 'all_containers_by_name_' is a private member of 'kudu::fs::LogBlock
Done
http://gerrit.cloudera.org:8080/#/c/7207/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 48: #include "kudu/util/locks.h"
> warning: #includes are not sorted properly [llvm-include-order]
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: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#7). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates which happen in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. A design doc can be found here: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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-stress-test.cc 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/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 18 files changed, 337 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/7 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#6). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates which happen in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. A design doc can be found here: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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-stress-test.cc 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/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 18 files changed, 338 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/6 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#5). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates/deletes which happen in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. Accumulating deletes can also allow more efficient holes punching. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. In this patch, it also intergates 'BlockTransaction' with flush/compaction operation and with orphaned blocks GC. A design doc can be found here: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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-stress-test.cc 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/tablet/tablet_metadata.cc 20 files changed, 453 insertions(+), 166 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/5 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/7207/3//COMMIT_MSG Commit Message: PS3, Line 10: which happen : in a logical op > 'which happen in a logical operation' Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/bloomfile.h File src/kudu/cfile/bloomfile.h: Line 47: // Close the bloom's CFile, finalizing the underlying blocks and > Update this comment. Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/cfile_writer.h File src/kudu/cfile/cfile_writer.h: Line 109: // Close the CFile. Finalize the underlying blocks and release > Update this comment. Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 67: // Close() is an expensive operation, as it must flush both dirty block data > This comment should be updated to explain Finalize. Done Line 142: virtual size_t BytesAppended() const = 0; > Add more info about what 'finalize' means in this context, either here or i Done Line 143: > Perhaps we could simplify this interface by combining 'FlushDataAsync' and yes, there is some cases the block is flushed but not finalized, you can call FlushDataAsync() and then Close(). Or it is finalized but not flushed, in cfile writer, if FLAGS_cfile_do_on_finish is 'nothing', the block will not be flushed. Line 144: virtual State state() const = 0; > On the naming of 'FinalizeWrite' - the name is good, but I'd be in favor of Done Line 283: // nor is the order guaranteed to be deterministic. Furthermore, if > Update comment Done Line 305: STLDeleteElements(&created_blocks_); > Hmm I know we discussed on slack (or the design doc?) why CommitWrites need We discussed that in flush/compaction, superblock gets flushed in between the new rowsets get created and the old rowsets get GCed. We do not want to delete blocks that should be kept, if superblock flush fails. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 429: string metadata_path = path + LogBlockManager::kContainerMetadataFileSuffix; > Is this needed? Done Line 915: unique_ptr block; > Should this be committing the writes? Seems strange the previous version d Looking at the purpose of the tests, the writes seem not necessarily need to be committed. So I keep it the way it was. Line 1107: unique_ptr block; > Same here. Same reason above. http://gerrit.cloudera.org:8080/#/c/7207/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 24: #include > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 274: > Any reason to prefer a set here to a vector? Vectors are typically lighter Was trying to only have unique block here. But given the way how this method gets called the blocks should all be unique, so will change to vector instead. Line 367: // the container data file's position, as round up could already be done via > Can't this method internally determine whether the round up has already bee Good point, but I feel it is a little bit harder than that, as we also have state as 'FLUSHING'. Also, LogBlock does not have the state info. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: Line 90: // Closes the CFiles, finalizing the underlying blocks and releasing > Update comment. 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: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#4). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates/deletes which happen in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. In this patch, it also intergates 'BlockTransaction' with flush/compaction operation. A design doc can be found here: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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 18 files changed, 407 insertions(+), 150 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/4 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Dan Burkert has posted comments on this change.
Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager
..
Patch Set 3:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/7207/3//COMMIT_MSG
Commit Message:
PS3, Line 10: happen in a
: logic operation
'which happen in a logical operation'
Line 20:
Add a link to the design doc.
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/bloomfile.h
File src/kudu/cfile/bloomfile.h:
Line 47: // Close the bloom's CFile, releasing the underlying block to
'closer'.
Update this comment.
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/cfile_writer.h
File src/kudu/cfile/cfile_writer.h:
Line 109: // Close the CFile and release the underlying writable block to
'closer'.
Update this comment.
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
Line 67: // Close() is an expensive operation, as it must flush both dirty
block data
This comment should be updated to explain Finalize.
Line 142: // Finalize a fully written block.
Add more info about what 'finalize' means in this context, either here or in
the class doc.
Line 143: virtual Status FinalizeWrite() = 0;
Perhaps we could simplify this interface by combining 'FlushDataAsync' and
'FinalizeWrite'. Put another way - do we always want to do these operations
together, or are there cases where one is done without the other?
Line 144:
On the naming of 'FinalizeWrite' - the name is good, but I'd be in favor of
shortening to 'Finalize'. I think the 'Write' part is understood given the
context of a WritableBlock.
Line 283: // Blocks must be closed explicitly via CloseBlocks(), otherwise they
will
Update comment
Line 305: Status CommitWrites() {
Hmm I know we discussed on slack (or the design doc?) why CommitWrites needs to
be separate of CommitDeletes, but I've lost that context now. Might be good to
explain why there split as part of the method comments.
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
Line 429:bm_->all_containers_by_name_.size();
Is this needed?
Line 915: BlockTransaction transaction;
Should this be committing the writes? Seems strange the previous version
didn't either.
Line 1107: BlockTransaction transaction;
Same here.
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 274: Status FinishBlocks(std::set blocks);
Any reason to prefer a set here to a vector? Vectors are typically lighter
weight.
Line 367: // block is fully written. 'should_update' indicates if it should
round up
Can't this method internally determine whether the round up has already been
done based on whether the block's state is DIRTY or FINALIZED?
http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/tablet/diskrowset.h
File src/kudu/tablet/diskrowset.h:
Line 90: // Closes the CFiles, releasing the underlying blocks to 'closer'.
Update comment.
--
To view, visit http://gerrit.cloudera.org:8080/7207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#3). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates/deletes happen in a logic operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. In this patch, it also intergates 'BlockTransaction' with flush/compaction operation. Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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/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 17 files changed, 290 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/3 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#2). Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates/deletes happen in a logic operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. In this patch, it also intergates 'BlockTransaction' with flush/compaction operation. Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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/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 17 files changed, 289 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/2 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7207/1/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 318: // TODO(Hao): complete CommitDeletes(). > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/7207/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 374: // Returns a block's length aligned to the nearest filesystem block size. > warning: parameter 'block_offset' is const-qualified in the function declar Done Line 375: int64_t fs_aligned_length(int64_t block_offset, > warning: parameter 'block_length' is const-qualified in the function declar Done Line 379: // of a fully written block. > warning: parameter 'block_length' is const-qualified in the function declar Done Line 379: // of a fully written block. > warning: parameter 'block_offset' is const-qualified in the function declar 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: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has uploaded a new change for review. http://gerrit.cloudera.org:8080/7207 Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager .. WIP KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate creates/deletes happen in a logic operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API FinalizeWrite() to Block Manager, so that for LBM container can be resued, without flushing in-flight writable blocks to disk. In this patch, it also intergates 'BlockTransaction' with flush/compaction operation. Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- 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-stress-test.cc 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/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 18 files changed, 292 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/1 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao
