You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2017/09/26 21:28:36 UTC
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8144
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that in
future they can be extended for specific implementations.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
17 files changed, 180 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/1
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#12).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 340 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/12
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 6:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG@10
PS4, Line 10: so that in
: future they can be extended for specific implementations.
> Is one of the motivations also to force block creation and deletion to take
When working on this patch, honestly speaking I do not have the intention to completely remove CreateBlock() and DeleteBlock(). But giving more thought doing so may simplify BM to some degree. If that is also your thought, I will update the comment accordingly.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc
File src/kudu/cfile/bloomfile.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc@116
PS4, Line 116: atus BloomFileWr
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc@937
PS4, Line 937: ASSERT_OK(w.Start());
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc@206
PS4, Line 206: TRACE_EVENT0("cfile", "CFileWriter::Finish");
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h@258
PS5, Line 258: t by this block man
> Nit: how about NewCreationTransaction()? And NewDeletionTransaction?
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@283
PS4, Line 283: // On success, guarantees that outstanding data is durable.
> I think this and BlockDeletionTransaction should be pure interfaces. It may
Makes sense, previously my concern was about having duplicate code, but I agree with the cons of not doing so.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325
PS4, Line 325:
> The rule of three explanation makes sense, but it might be nice to define a
Sure, will update.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc@801
PS4, Line 801: RETURN_NOT_OK_HANDLE_DISK_FAILURE((status_expr), \
: error_manager_->RunErrorNotificationCb(dd_manager_->FindDataDirByUuidIndex( \
: internal::FileBlockLocatio
> Nit: combine
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc@1832
PS4, Line 1832:
: // Deletion is forbidden for read-only container.
: scoped_refptr<LogBlock> lb;
> Nit: combine
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc@116
PS4, Line 116: Status DeltaFileWriter::Finish() {
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc@231
PS4, Line 231: TRACE_EVENT0("tablet", "DiskRowSetWriter::Finish");
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc
File src/kudu/tablet/multi_column_writer.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc@123
PS4, Line 123: BlockManager* bm = fs_->block_manager();
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tserver/tablet_copy_client.cc@160
PS4, Line 160: tablet_copy_metrics_(tablet_copy_metrics) {
> We having "using fs::..." stuff above; you can do the same for BlockManager
Done
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 05 Oct 2017 17:25:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#9).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 335 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/9
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#10).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 336 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/10
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#8).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 328 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/8
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152
PS8, Line 1152: created_blocks_.emplace_back(unique_ptr<LogWritableBlock>(lwb));
> I understand what you mean now; thanks for the explanation. Yes, the old ap
Done
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 11 Oct 2017 00:51:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Reviewed-on: http://gerrit.cloudera.org:8080/8144
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 340 insertions(+), 177 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h@285
PS11, Line 285: a transacti
> Nit: "a transaction"
Done
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h@287
PS11, Line 287: thr
> Nit: Don't need "the".
Done
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h@292
PS11, Line 292: // Add a block to the creation transaction.
> You should still have a brief doc for Add, it just doesn't have to talk abo
Done
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 11 Oct 2017 22:57:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#6).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that in
future they can be extended for specific implementations.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 347 insertions(+), 176 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/6
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG@10
PS4, Line 10: so that in
: future they can be extended for specific implementations.
> When working on this patch, honestly speaking I do not have the intention t
Yeah I think it's a good idea. I recall discussing that with you and Dan in one of our meetings a few months ago. The LBM code is complicated enough as it is so anything that simplifies it by reducing the number of code paths is welcome.
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/block_manager.h@286
PS6, Line 286: virtual const std::vector<std::unique_ptr<WritableBlock>>& created_blocks() const = 0;
This isn't present in BlockDeletionTransaction and, AFAICT, is only used in tests. Can it be removed?
Removing this will allow LogBlockCreationTransaction to move the down_cast of WritableBlock from CommitCreatedBlocks() to AddCreatedBlock(), which will make CommitCreatedBlocks() a little simpler. And maybe it'll allow other simplifications too.
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 00:14:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#2).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that in
future they can be extended for specific implementations.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
17 files changed, 179 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/2
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG@10
PS4, Line 10: so that
: they can be extended for specific implementations. Moreov
> Yeah I think it's a good idea. I recall discussing that with you and Dan in
Sounds good, will update.
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/block_manager.h@286
PS6, Line 286:
> This isn't present in BlockDeletionTransaction and, AFAICT, is only used in
Done. But I removed certain test code, because I don't see a clean way to access created_blocks from the cfile_test. Open to your suggestions? :)
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 07:00:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/file_block_manager.cc@856
PS6, Line 856: return std::make_shared<internal::FileBlockDeletionTransaction>(this);
> Why did you remove these checks?
Yeah, I removed these checks for the reason you figure out at in the following patch. :) I will add it back and make changes in the following patches based on your recommendations accordingly.
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152
PS8, Line 1152: created_blocks_.emplace_back(unique_ptr<LogWritableBlock>(lwb));
> Now that created_blocks() is gone, you could store these directly into some
Done, but I kept "std::vector<std::unique_ptr<LogWritableBlock>> created_blocks_;", because using 'unordered_map<LogBlockContainer*, vector<unique_ptr<LogWritableBlock>>> created_block_map_;' only means more transformation for L1160, L1162 and L1176. Please let me know if you think this makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 10 Oct 2017 22:41:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#11).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future.
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 338 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/11
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8144/10/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/10/src/kudu/fs/block_manager.h@289
PS10, Line 289: // Add a block to the creation transaction. This method is not thread-safe.
> I don't think it's appropriate for threads to share BlockCreationTransactio
Yeah, makes sense. I was in between to document it in the method level or class level. :) Will update.
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152
PS8, Line 1152: std::unique_ptr<WritableBlock> block) {
> Well, both pieces of state is definitely worse than just one, so I'd rather
The issue with the third is DoCloseBlocks() takes 'const vector<LogWritableBlock*>&' as parameter and created_block_map stores 'vector<std::unique_ptr<LogWritableBlock>>'. So that means we need to either do the transformation or change the input parameter AFAICT. I don't think doing the later is a good idea, since we are not changing ownership in DoCloseBlocks() and DoCloseBlocks() is called in other places as well.
I agree with you that the first and the second changes doesn't seem to be big deals. But adding all of these together, may not better that keeping it as the way it was? Honestly speaking, I personally prefer to revert to the old approach.
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 10 Oct 2017 23:41:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325
PS4, Line 325: explicit BlockDeletionTransaction(BlockManager* bm)
Should this have a virtual dtor as well?
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:52:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8144/10/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/10/src/kudu/fs/block_manager.h@289
PS10, Line 289: // Add a block to the creation transaction. This method is not thread-safe.
I don't think it's appropriate for threads to share BlockCreationTransaction or BlockDeletionTransaction. Each thread should use its own transaction, or use external synchronization to safely share a single instance. So, rather than calling out just the Add methods as being thread unsafe, could you document that the classes themselves aren't thread safe?
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152
PS8, Line 1152: std::unique_ptr<WritableBlock> block) {
> Done, but I kept "std::vector<std::unique_ptr<LogWritableBlock>> created_bl
Well, both pieces of state is definitely worse than just one, so I'd rather revert to the old approach or figure out what's wrong with the new one:
1. L1160 is cosmetic; it could just as easily VLOG about the number of containers with commited blocks.
2. L1162 can be changed to a double for loop to iterate on every block. Not a big deal.
3. What's the additional transformation needed on L1176? It's already iterating over created_block_map; am I missing something?
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 10 Oct 2017 22:59:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 12 Oct 2017 00:01:55 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 5:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG@10
PS4, Line 10: so that in
: future they can be extended for specific implementations.
Is one of the motivations also to force block creation and deletion to take place via these transactions rather than today's CreateBlock() and DeleteBlock()? If so, it would be good to understand how we'll get there, since that requires some additional refactoring.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc
File src/kudu/cfile/bloomfile.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc@116
PS4, Line 116: fs::BlockManager
We having "using fs::..." stuff above; you can do the same for BlockManager.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc@937
PS4, Line 937: fs::BlockManager* bm = fs_manager_->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc@206
PS4, Line 206: fs::BlockManager* bm = block_->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.
http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h@258
PS5, Line 258: CreationTransaction
Nit: how about NewCreationTransaction()? And NewDeletionTransaction?
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@283
PS4, Line 283: class BlockCreationTransaction {
I think this and BlockDeletionTransaction should be pure interfaces. It may result in some duplicated code (not much, I think), but I believe it'll let you eliminate BlockManager::CloseBlocks, which AFAICT only exists because BlockCreationTransaction isn't a member of LogBlockManager.
The other problem with the transaction classes being concrete classes is that there's nothing stopping me from declaring and using them directly, when the intent of this patch is (I believe) to force people to use CreationTransaction() and DeletionTransaction(). Is this going to be addressed in a follow-on patch?
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325
PS4, Line 325: explicit BlockDeletionTransaction(BlockManager* bm)
> I read it at: http://en.cppreference.com/w/cpp/language/rule_of_three that
The rule of three explanation makes sense, but it might be nice to define a virtual dtor just so this is more consistent with BlockCreationTransaction. It's easy to look at the two classes, see that one doesn't define a virtual dtor, and assume it's a mistake.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc@801
PS4, Line 801: unique_ptr<internal::FileBlockCreationTransaction> transaction(
: new internal::FileBlockCreationTransaction(this));
: return std::move(transaction);
Nit: combine
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc@1832
PS4, Line 1832: unique_ptr<internal::LogBlockCreationTransaction> transaction(
: new internal::LogBlockCreationTransaction(this));
: return std::move(transaction);
Nit: combine
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc@116
PS4, Line 116: fs::BlockManager* bm = writer_->block()->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc@231
PS4, Line 231: fs::BlockManager* bm = rowset_metadata_->fs_manager()->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc
File src/kudu/tablet/multi_column_writer.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc@123
PS4, Line 123: unique_ptr<BlockCreationTransaction> transaction = bm->CreationTransaction();
We having "using fs::..." stuff above; you can do the same for BlockManager.
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tserver/tablet_copy_client.cc@160
PS4, Line 160: fs::BlockManager* bm = fs_manager->block_manager();
We having "using fs::..." stuff above; you can do the same for BlockManager.
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:50:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325
PS4, Line 325: explicit BlockDeletionTransaction(BlockManager* bm)
> Should this have a virtual dtor as well?
I read it at: http://en.cppreference.com/w/cpp/language/rule_of_three that virtual destructor can be 'avoided if the objects of the derived class are not dynamically allocated, or are dynamically allocated only to be stored in a std::shared_ptr (such as by std::make_shared): shared pointers invoke the derived class destructor even after casting to std::shared_ptr<Base>.' Since BlockDeletionTransaction and its subclasses will only be used with shared_ptr, I think it is ok to not define virtual destructor here?
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 19:24:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/6/src/kudu/fs/file_block_manager.cc@856
PS6, Line 856: return std::make_shared<internal::FileBlockDeletionTransaction>(this);
Why did you remove these checks?
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152
PS8, Line 1152: created_blocks_.emplace_back(unique_ptr<LogWritableBlock>(lwb));
Now that created_blocks() is gone, you could store these directly into something like created_block_map, to avoid the transformation that happens in CommitCreatedBlocks().
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Oct 2017 22:17:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8144
to look at the new patch set (#7).
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
This patch refactors both BlockCreationTransaction and
BlockDeletionTransaction to be created from block manager, so that
they can be extended for specific implementations. Moreover,
we may be able to completely remove BlockManager's CreateBlock() and
DeleteBlock() to simplify the code paths in future..
Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
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/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
18 files changed, 328 insertions(+), 177 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8144/7
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h@285
PS11, Line 285: transaction
Nit: "a transaction"
Below too.
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h@287
PS11, Line 287: the
Nit: Don't need "the".
Below too.
http://gerrit.cloudera.org:8080/#/c/8144/11/src/kudu/fs/block_manager.h@292
PS11, Line 292: virtual void AddCreatedBlock(std::unique_ptr<WritableBlock> block) = 0;
You should still have a brief doc for Add, it just doesn't have to talk about thread safety.
Below too.
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:35:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152
PS8, Line 1152: std::unique_ptr<WritableBlock> block) {
> The issue with the third is DoCloseBlocks() takes 'const vector<LogWritable
I understand what you mean now; thanks for the explanation. Yes, the old approach makes more sense.
--
To view, visit http://gerrit.cloudera.org:8080/8144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6
Gerrit-Change-Number: 8144
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 10 Oct 2017 23:43:45 +0000
Gerrit-HasComments: Yes