You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2022/07/06 16:46:41 UTC
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18709
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Refactor LogBlockManager as a base class, add LogfBlockManager extend
from it.
LogfBlockManager is the Log Block Manager which manage the append only
file to store containers' metadata, it is how we do as before.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 46 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/1
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#10).
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager a base class
This patch makes LogBlockManager a base class for the newly
added LogBlockManagerNativeMeta.
FileMetaLogBlockManager is the log-backed block manager which
manages the sequential written file to store containers' metadata,
it is how we do as before.
Most of the member functions are left in the base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
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.h
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
8 files changed, 90 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/10
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18709/7/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/7/src/kudu/fs/log_block_manager.h@477
PS7, Line 477: LogBlockManagerMetadataType type_;
> This change is very small, and it make the super class and the derived clas
Done
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 12:31:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@178
PS8, Line 178: override;
Also reported by tidy like:
> warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sun, 08 Jan 2023 16:22:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@180
PS8, Line 180: Status Open(FsReport* report, std::atomic<int>* containers_processed,
: std::atomic<int>* containers_total) override;
> The Tidy Bot report an error on PS1, so I changed it.
Fixed these issues by https://gerrit.cloudera.org/c/19406/
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 09 Jan 2023 06:35:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#2).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Refactor LogBlockManager as a base class, add LogfBlockManager extend
from it.
LogfBlockManager is the Log Block Manager which manage the append only
file to store containers' metadata, it is how we do as before.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
5 files changed, 50 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/2
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#4).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Refactor LogBlockManager as a base class, add LogfBlockManager extend
from it.
LogfBlockManager is the Log Block Manager which manage the append only
file to store containers' metadata, it is how we do as before.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 63 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/4
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#9).
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager a base class
This patch makes LogBlockManager a base class for the newly
added LogBlockManagerNativeMeta.
FileMetaLogBlockManager is the log-backed block manager which
manages the sequential written file to store containers' metadata,
it is how we do as before.
Most of the member functions are left in the base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
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.h
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
8 files changed, 89 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/9
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18709/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18709/7//COMMIT_MSG@11
PS7, Line 11: FileMetaLogBlockManageris
nit: FileMetaLogBlockManager is
http://gerrit.cloudera.org:8080/#/c/18709/7/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/7/src/kudu/fs/log_block_manager.h@477
PS7, Line 477: LogBlockManagerMetadataType type_;
It seems 'type_' has only one value 'kSequentialWrittenFile' now.
And the 'type_' seems useless at the patch except a DCHECK_EQ.
In future's patch, 'rocksdb' type may be added, add it at that patch?
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 16:27:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
Patch Set 13:
Let's wait and merge it to master branch until 1.17 branch been cut.
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Thu, 02 Feb 2023 07:54:17 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
Patch Set 8:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG
Commit Message:
PS8:
FileMetaLogBlockManager looks confusing because of already existing FileBlockManager
I'd suggest naming the new class LogBlockManagerNativeMeta or something like that.
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@7
PS8, Line 7: make LogBlockManager as a base class
make LogBlockManager a base class
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@9
PS8, Line 9: This patch makes LogBlockManager as a base class, and adds
: FileMetaLogBlockManager inherit from it
This patch makes LogBlockManager a base class for the newly added LogBlockManagerNativeMeta.
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@12
PS8, Line 12: manage
manages
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15
PS8, Line 15: member functions
the member functions
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15
PS8, Line 15: base
the base
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@16
PS8, Line 16: to avoid too large updating on existing code, I'm planning to do
: that in next patches.
I thought it would be easier to separate those private/protected methods as per their current usage in the new hierarchy in this patch, no?
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@178
PS8, Line 178: override;
I guess 'override' or 'virtual' here isn't needed: the base BlockManager class has already introduced the virtual base since its destructor defined as virtual
virtual ~BlockManager() {}
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@180
PS8, Line 180: Status Open(FsReport* report, std::atomic<int>* containers_processed,
: std::atomic<int>* containers_total) override;
Once the signature has changed, it's no longer an override for BlockManager::Open(). This method now shadows BlockManager::Open(), but it doesn't override it.
Is that intentional? If yes, then that's not a good practice anyways.
BTW, what's wrong with the original signature of the method? Why to change it?
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@200
PS8, Line 200: FRIEND_TEST(LogBlockManagerTest, TestAbortBlock);
: FRIEND_TEST(LogBlockManagerTest, TestCloseFinalizedBlock);
: FRIEND_TEST(LogBlockManagerTest, TestCompactFullContainerMetadataAtStartup);
: FRIEND_TEST(LogBlockManagerTest, TestFinalizeBlock);
: FRIEND_TEST(LogBlockManagerTest, TestLIFOContainerSelection);
: FRIEND_TEST(LogBlockManagerTest, TestLookupBlockLimit);
: FRIEND_TEST(LogBlockManagerTest, TestMetadataTruncation);
: FRIEND_TEST(LogBlockManagerTest, TestMisalignedBlocksFuzz);
: FRIEND_TEST(LogBlockManagerTest, TestParseKernelRelease);
: FRIEND_TEST(LogBlockManagerTest, TestBumpBlockIds);
: FRIEND_TEST(LogBlockManagerTest, TestReuseBlockIds);
: FRIEND_TEST(LogBlockManagerTest, TestFailMultipleTransactionsPerContainer);
:
: friend class internal::LogBlockContainer;
: friend class internal::LogBlockDeletionTransaction;
: friend class internal::LogWritableBlock;
: friend class LogBlockManagerTest;
I don't think this need to be in the 'protected' part, can be in private part easily.
That's also true for many other fields and methods that were private in prior version even if FileMetaLogBlockManager has appeared.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@472
PS8, Line 472: Where the metadata to store
'Where to store the metadata' or 'Where the metadata is stored'
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@473
PS8, Line 473: enum LogBlockManagerMetadataType {
Why to introduce this enumeration at all? Isn't it enough to have a set of virtual methods in the interface to deal with the specifics of the metadata storage specifics?
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474
PS8, Line 474: sequential
sequentially
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474
PS8, Line 474: into
in
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474
PS8, Line 474: i.e.
e.g.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@475
PS8, Line 475: kSequentialWrittenFile
kSequentiallyWrittenFile
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@487
PS8, Line 487: // The metadata part of a container is written sequentially to a metadata file.
Please add a more generic comment to express the essence of this class.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@489
PS8, Line 489: even after a great many
even after many
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Thu, 05 Jan 2023 03:12:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#8).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class
This patch makes LogBlockManager as a base class, and adds
FileMetaLogBlockManager inherit from it.
FileMetaLogBlockManager is the log-backed block manager which
manage the sequential written file to store containers' metadata,
it is how we do as before.
Most of member functions are left in base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 90 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/8
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
Patch Set 8: Code-Review+1
> Patch Set 8:
>
> (1 comment)
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 12:31:40 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18709/7/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/7/src/kudu/fs/log_block_manager.h@477
PS7, Line 477: LogBlockManagerMetadataType type_;
> It seems 'type_' has only one value 'kSequentialWrittenFile' now.
This change is very small, and it make the super class and the derived class more meaningful.
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sat, 17 Dec 2022 02:11:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#13).
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager a base class
This patch makes LogBlockManager a base class for the newly
added LogBlockManagerNativeMeta.
FileMetaLogBlockManager is the log-backed block manager which
manages the sequential written file to store containers' metadata,
it is how we do as before.
Most of the member functions are left in the base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 69 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/13
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18709/1/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/1/src/kudu/fs/log_block_manager.h@188
PS1, Line 188: ~LogBlockManager() override;
> warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [
Done
http://gerrit.cloudera.org:8080/#/c/18709/1/src/kudu/fs/log_block_manager.h@190
PS1, Line 190: Status Open(FsReport* report, std::atomic<int>* containers_processed,
> warning: default arguments on virtual or override methods are prohibited [g
Done
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sat, 30 Jul 2022 07:52:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#7).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class
This patch makes LogBlockManager as a base class, and adds
FileMetaLogBlockManager inherit from it.
FileMetaLogBlockManageris the log-backed block manager which
manage the sequential written file to store containers' metadata,
it is how we do as before.
Most of member functions are left in base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 90 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/7
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#5).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class
This patch makes LogBlockManager as a base class, and adds
FileMetaLogBlockManager inherit from it.
FileMetaLogBlockManageris the log-backed block manager which
manage the sequential written file to store containers' metadata,
it is how we do as before.
Most of member functions are left in base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 78 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/5
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18709/9/src/kudu/fs/file_block_manager.h
File src/kudu/fs/file_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/9/src/kudu/fs/file_block_manager.h@81
PS9, Line 81: ~FileBlockManager() override;
> warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [
Done
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 09 Jan 2023 00:34:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
Patch Set 13: Code-Review+2
(2 comments)
Thank you for working on this!
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@178
PS8, Line 178: override;
> Also reported by tidy like:
That makes sense, thanks!
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@473
PS8, Line 473: // Metrics for the block manager.
> It's useful in next patches but now seems useless, as Yuqi also confused ab
Sure! Let's see if we can get away without introducing these :)
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Thu, 02 Feb 2023 02:13:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager a base class
This patch makes LogBlockManager a base class for the newly
added LogBlockManagerNativeMeta.
FileMetaLogBlockManager is the log-backed block manager which
manages the sequential written file to store containers' metadata,
it is how we do as before.
Most of the member functions are left in the base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Reviewed-on: http://gerrit.cloudera.org:8080/18709
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 69 insertions(+), 46 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#3).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class to be more extendable
Refactor LogBlockManager as a base class, add LogfBlockManager extend
from it.
LogfBlockManager is the Log Block Manager which manage the append only
file to store containers' metadata, it is how we do as before.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 58 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/3
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#6).
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager as a base class
This patch makes LogBlockManager as a base class, and adds
FileMetaLogBlockManager inherit from it.
FileMetaLogBlockManageris the log-backed block manager which
manage the sequential written file to store containers' metadata,
it is how we do as before.
Most of member functions are left in base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.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
6 files changed, 87 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/6
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3371 [fs] make LogBlockManager as a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class
......................................................................
Patch Set 6:
> Patch Set 5:
>
> (1 comment)
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 12 Dec 2022 08:21:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
Patch Set 8:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG
Commit Message:
PS8:
> FileMetaLogBlockManager looks confusing because of already existing FileBlo
Done
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@7
PS8, Line 7: make LogBlockManager as a base class
> make LogBlockManager a base class
Done
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@9
PS8, Line 9: This patch makes LogBlockManager as a base class, and adds
: FileMetaLogBlockManager inherit from it
> This patch makes LogBlockManager a base class for the newly added LogBlockM
Done
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@12
PS8, Line 12: manage
> manages
Done
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15
PS8, Line 15: member functions
> the member functions
Done
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15
PS8, Line 15: base
> the base
Done
http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@16
PS8, Line 16: to avoid too large updating on existing code, I'm planning to do
: that in next patches.
> I thought it would be easier to separate those private/protected methods as
Yes, I reverted some modifications.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@178
PS8, Line 178: override;
Yes it's not needed, but maybe help to serve as documentation. I find the Google C++ code style guide suggest to add override for destructors.
> Explicitly annotate overrides of virtual functions or virtual destructors with exactly one of an override or (less frequently) final specifier. Do not use virtual when declaring an override. Rationale: A function or destructor marked override or final that is not an override of a base class virtual function will not compile, and this helps catch common errors. The specifiers serve as documentation; if no specifier is present, the reader has to check all ancestors of the class in question to determine if the function or destructor is virtual or not.
And there is an interesting discussion about it https://github.com/isocpp/CppCoreGuidelines/issues/721, so IMO keep the 'override' maybe help for readers.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@180
PS8, Line 180: Status Open(FsReport* report, std::atomic<int>* containers_processed,
: std::atomic<int>* containers_total) override;
The Tidy Bot report an error on PS1, so I changed it.
> warning: default arguments on virtual or override methods are prohibited [google-default-arguments]
It report it because I'm happen to modify this file.
Maybe separate these kind of refactors to another patch would be better.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@200
PS8, Line 200: FRIEND_TEST(LogBlockManagerTest, TestAbortBlock);
: FRIEND_TEST(LogBlockManagerTest, TestCloseFinalizedBlock);
: FRIEND_TEST(LogBlockManagerTest, TestCompactFullContainerMetadataAtStartup);
: FRIEND_TEST(LogBlockManagerTest, TestFinalizeBlock);
: FRIEND_TEST(LogBlockManagerTest, TestLIFOContainerSelection);
: FRIEND_TEST(LogBlockManagerTest, TestLookupBlockLimit);
: FRIEND_TEST(LogBlockManagerTest, TestMetadataTruncation);
: FRIEND_TEST(LogBlockManagerTest, TestMisalignedBlocksFuzz);
: FRIEND_TEST(LogBlockManagerTest, TestParseKernelRelease);
: FRIEND_TEST(LogBlockManagerTest, TestBumpBlockIds);
: FRIEND_TEST(LogBlockManagerTest, TestReuseBlockIds);
: FRIEND_TEST(LogBlockManagerTest, TestFailMultipleTransactionsPerContainer);
:
: friend class internal::LogBlockContainer;
: friend class internal::LogBlockDeletionTransaction;
: friend class internal::LogWritableBlock;
: friend class LogBlockManagerTest;
> I don't think this need to be in the 'protected' part, can be in private pa
That's true, I reverted it, and kept the minimal protected scope.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@472
PS8, Line 472: Where the metadata to store
> 'Where to store the metadata' or 'Where the metadata is stored'
Done
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@473
PS8, Line 473: enum LogBlockManagerMetadataType {
> Why to introduce this enumeration at all? Isn't it enough to have a set of
It's useful in next patches but now seems useless, as Yuqi also confused about it.
Let's add it back in following up patches if it's needed.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474
PS8, Line 474: into
> in
Done
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474
PS8, Line 474: sequential
> sequentially
Done
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474
PS8, Line 474: i.e.
> e.g.
Done
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@475
PS8, Line 475: kSequentialWrittenFile
> kSequentiallyWrittenFile
Done
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@487
PS8, Line 487: // The metadata part of a container is written sequentially to a metadata file.
> Please add a more generic comment to express the essence of this class.
Improved, but I'm not sure if it is appropriate.
http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@489
PS8, Line 489: even after a great many
> even after many
Done
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sun, 08 Jan 2023 16:21:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 [fs] make LogBlockManager a base class
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18709
to look at the new patch set (#12).
Change subject: KUDU-3371 [fs] make LogBlockManager a base class
......................................................................
KUDU-3371 [fs] make LogBlockManager a base class
This patch makes LogBlockManager a base class for the newly
added LogBlockManagerNativeMeta.
FileMetaLogBlockManager is the log-backed block manager which
manages the sequential written file to store containers' metadata,
it is how we do as before.
Most of the member functions are left in the base class LogBlockManager
to avoid too large updating on existing code, I'm planning to do
that in next patches.
Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
---
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.h
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
8 files changed, 90 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/18709/12
--
To view, visit http://gerrit.cloudera.org:8080/18709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356
Gerrit-Change-Number: 18709
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>