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 2021/09/14 07:33:35 UTC
[kudu-CR] [fs] Refactor fs module
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17845
Change subject: [fs] Refactor fs module
......................................................................
[fs] Refactor fs module
Only some code style refactoring on fs module, without
any functional changes.
Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
---
M src/kudu/fs/block_id.cc
M src/kudu/fs/dir_manager.cc
M src/kudu/fs/dir_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/fs_report.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/server/server_base.cc
10 files changed, 91 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/17845/1
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [fs] Refactor fs module
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc
File src/kudu/fs/fs_report.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@42
PS1, Line 42: MERGE_ENTRIE
> If you are OK with the smell of this macro relying on the naming of 'entrie
This macro is more or less like a member function of current structure, so I think access this structure's member variable 'entries' is not needed to pass as parameter. :)
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 16 Sep 2021 02:52:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs] Refactor fs module
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc
File src/kudu/fs/fs_report.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@126
PS1, Line 126: int64_t full_container_space_count_repaired = 0;
: int64_t full_container_space_bytes_repaired = 0;
: int64_t full_container_space_bytes = 0;
nit: Probably we can have the same order of initialization for orphaned_blocks* (above in line 91-93) and full_container_space* for uniformity?
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 15 Sep 2021 15:38:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs] Refactor fs module
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.h
File src/kudu/fs/dir_manager.h:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.h@254
PS1, Line 254: AreAllDirsFa
> Rename to AllDirsFailed() or AreAllDirsFailed()
Done
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.cc
File src/kudu/fs/dir_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.cc@227
PS1, Line 227: bool has_healthy_instances = true;
> Just curious: why to assign any value here if LoadInstances() do not return
It is used to mute a tidy warning: https://gerrit.cloudera.org/c/17837/1/src/kudu/fs/dir_manager.cc#514
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc
File src/kudu/fs/fs_report.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@42
PS1, Line 42: MERGE_ENTRIE
> nit: rename to MERGE_ENTRIES and introduce the 'entries' parameter as well?
I'd like to rename it to 'MERGE_ENTRIES_FROM', 'ENTRIES' is implicit in macro name and 'FROM' means merge with some 'other'.
Introduce both 'entries' and 'other' will make the macro expansion a little smelly IMO.
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@126
PS1, Line 126: int64_t full_container_space_bytes = 0;
: int64_t full_container_space_count_repaired = 0;
: int64_t full_container_space_bytes_repa
> nit: Probably we can have the same order of initialization for orphaned_blo
Done
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/log_block_manager.cc@2992
PS1, Line 2992: const string dir = containe
> nit: if you moved it closer to the site of its usage, maybe move it one mor
Line 'const string dir = container.data_dir()->dir();' is not used too, I've removed it.
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc@584
PS1, Line 584: LOG(INFO) << "This appears to be a new deployment of Kudu; creating new FS layout";
: is_first_run_ = true;
> nit: combine these two INFO messages into one?
Emmm, in fact this is a small fix. Some one removed a corrupt .metadata file of a log block container on a tserver, then the tserver start failed, and hit this 'not found' banch without any useful log, it cost me much time to trouble shooting.
I think this 'not found' branch is only for instance files not found, not for other type of 'not found' (e.g. a corresponding .metadata file not found). A better way is to return some other type of Status. May fix it later.
I've removed this line and add another warning log when open Dir failed.
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Sep 2021 16:00:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs] Refactor fs module
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module
......................................................................
[fs] Refactor fs module
Only some code style refactoring on fs module, without
any functional changes.
Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Reviewed-on: http://gerrit.cloudera.org:8080/17845
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/fs/block_id.cc
M src/kudu/fs/dir_manager.cc
M src/kudu/fs/dir_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/fs_report.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 92 insertions(+), 100 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [fs] Refactor fs module
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17845
to look at the new patch set (#3).
Change subject: [fs] Refactor fs module
......................................................................
[fs] Refactor fs module
Only some code style refactoring on fs module, without
any functional changes.
Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
---
M src/kudu/fs/block_id.cc
M src/kudu/fs/dir_manager.cc
M src/kudu/fs/dir_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/fs_report.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 92 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/17845/3
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [fs] Refactor fs module
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17845
to look at the new patch set (#2).
Change subject: [fs] Refactor fs module
......................................................................
[fs] Refactor fs module
Only some code style refactoring on fs module, without
any functional changes.
Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
---
M src/kudu/fs/block_id.cc
M src/kudu/fs/dir_manager.cc
M src/kudu/fs/dir_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/fs_report.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
9 files changed, 92 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/17845/2
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [fs] Refactor fs module
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc
File src/kudu/fs/fs_report.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@42
PS1, Line 42: MERGE_ENTRIE
> I'd like to rename it to 'MERGE_ENTRIES_FROM', 'ENTRIES' is implicit in mac
If you are OK with the smell of this macro relying on the naming of 'entries' field in the corresponding structures, why not to go further and rely on the naming of the 'other' parameter in the corresponding MergeFrom() methods? Dropping the 'other' macro parameter and making it implicit as well (as it is for 'entries') would yield a macro without any parameters:
MERGE_ENTRIES_FROM_OTHER()
:)
It's just a nit anyways, so feel free keep it as is.
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc@584
PS1, Line 584: LOG(INFO) << "This appears to be a new deployment of Kudu; creating new FS layout";
: is_first_run_ = true;
> Emmm, in fact this is a small fix. Some one removed a corrupt .metadata fil
Yep, it seems the case you mentioned is rather commanding the usage of the Status::Corruption() instead of Status::NotFound() status.
I agree -- adding a warning into the exact site looks better.
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Sep 2021 17:37:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs] Refactor fs module
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.h
File src/kudu/fs/dir_manager.h:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.h@254
PS1, Line 254: AllDirFailed
Rename to AllDirsFailed() or AreAllDirsFailed()
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.cc
File src/kudu/fs/dir_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.cc@227
PS1, Line 227: bool has_healthy_instances = true;
Just curious: why to assign any value here if LoadInstances() do not return OK without setting the 'has_healthy_instances' output parameter?
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc
File src/kudu/fs/fs_report.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@42
PS1, Line 42: MERGE_CHECKS
nit: rename to MERGE_ENTRIES and introduce the 'entries' parameter as well? Otherwise, why to expose only the 'other' parameter out of the macro?
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/log_block_manager.cc@2992
PS1, Line 2992: uint64_t old_metadata_size;
nit: if you moved it closer to the site of its usage, maybe move it one more line down right before its usage in the call to env_->GetFileSize()?
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc@584
PS1, Line 584: LOG(INFO) << s.ToString();
: LOG(INFO) << "This appears to be a new deployment of Kudu; creating new FS layout";
nit: combine these two INFO messages into one?
--
To view, visit http://gerrit.cloudera.org:8080/17845
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc
Gerrit-Change-Number: 17845
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 14 Sep 2021 16:34:14 +0000
Gerrit-HasComments: Yes