You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "helifu (Code Review)" <ge...@cloudera.org> on 2019/01/18 17:15:17 UTC
[kudu-CR] [fs]: LBM moves half-present containers out of the way
helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12239
Change subject: [fs]: LBM moves half-present containers out of the way
......................................................................
[fs]: LBM moves half-present containers out of the way
this patch intends to modify the LBM to move half-present
container whose data file is missing, and the present
half has no live blocks (.metadata file).
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 91 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/1
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 4:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG@9
PS4, Line 9: This patch intends to delete the (orphaned) metadata file at
: startup while the data file has gone missing and the metadata
: file has no live blocks at the same time. In the previous patch
: KUDU-2636, it has supported deleting the dead container which
: includes data file and metadata file. So, there will be a time
: window while deleting both of these two files in sequential,
: and it will make a half-container in extreme cases, especially
: in a crash or sent the tserver a kill -9. Indeed, Adar has
: captured this type of case under 1000 runs of dense_node-itest.
Thanks for the added detail. Could you reword this a bit to add more clarity?
With KUDU-2636 fixed, we now support deleting dead containers at runtime.
Because this involves deleting a pair of files, there's a time window in which
it's possible for there to be just one file. A well-timed crash or kill -9 can
make this a permanent state that fails the log block manager at startup. Indeed,
a loop of dense_node-itest failed 2/1000 times in this way.
This patch fixes the problem by detecting "orphaned" metadata files at startup
and deleting them. A metadata file is orphaned if it has no corresponding data
file and if it contains nothing but dead blocks.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1896
PS4, Line 1896: const auto CheckRepaired = [&] () {
If you really want to test that the repair happened, you should look at the FsReport that's created when the block manager is opened, and test the LBMIncompleteContainerCheck within it.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1987
PS4, Line 1987: // Create a metadata file with 1 byte which is <MIN.
: unique_ptr<WritableFile> metadata_file_writer;
: ASSERT_OK(env_->NewWritableFile(metadata_file_name, &metadata_file_writer));
: ASSERT_OK(metadata_file_writer->Append(Slice("a")));
: metadata_file_writer->Close();
Since this pattern is repeated quite a few times, could you encapsulate it in a lambda?
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@2029
PS4, Line 2029: // Delete the only block.
: vector<BlockId> deleted;
: shared_ptr<BlockDeletionTransaction> transaction = bm_->NewDeletionTransaction();
: transaction->AddDeletedBlock(block_id);
: ASSERT_OK(transaction->CommitDeletedBlocks(&deleted));
: transaction.reset();
: // Wait for the actual hole punching to take place.
: for (const auto& data_dir : dd_manager_->data_dirs()) {
: data_dir->WaitOnClosures();
: }
Likewise, encapsulate this in a lambda.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774: *-------------*------------*----------------*-------------------------*---------------------*/
> In addition, the metadata file is created before data file in 'LogBlockCont
I don't think the order of which file is created/deleted matters because without an fsync() on the parent directory, the filesystem may reorder operations under the hood. A great reference on this is https://danluu.com/file-consistency (see the "Filesystem semantics" section); _some_ filesystems may reorder directory operations, though among ext4/xfs (the filesystems we generally support for Kudu) this doesn't appear to be a problem.
Since this doesn't appear to be a problem on ext4/xfs, we can cross this bridge when it becomes an actual issue.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801
PS1, Line 801: return Status::OK();
> Our initial goal was to cover scenarios when the data file has gone missing
If you look at all the filesystem calls made in this file, they're wrapped in macros like RETURN_NOT_OK_CONTAINER_DISK_FAILURE that capture failed I/O and convert it into a disk failure. Because disks can fail at any time and for any reason, I think it's important to ensure that this wrapping is 100% inclusive. In your patch, if the disk fails during ReadNextPB, we won't notice that.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@762
PS4, Line 762: /* Note: the status here only represents the result of check.
Could you reformat this to use multi-line C++ comments?
// first line
// second line
// ...
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@767
PS4, Line 767: EXIST && NO LIVE BLCOKS | EXIST && LIVE BLCOKS|
BLOCKS
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@769
PS4, Line 769: (new case)
Don't need this; if you really want to distinguish between the old and new cases, do so descriptively (i.e. "Aborted (orphaned metadata)" vs. "Aborted (no data and too short metadata)", etc.)
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@830
PS4, Line 830: report->incomplete_container_check->entries.emplace_back(common_path);
: return Status::Aborted(Substitute("orphaned empty metadata and data files $0", common_path));
Nit: looks like these lines are indented too much.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@839
PS4, Line 839: if (PREDICT_FALSE(s_data.IsNotFound())) {
: if (metadata_size >= pb_util::kPBContainerMinimumValidLength) {
Can combine these and save one level of indentation.
Probably clearer to switch the order too (more symmetry with the condition above):
if (PREDICT_FALSE(metadata_size >= pb_util::kPBContainerMinimumValidLength &&
s_data.IsNotFound()))
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@863
PS4, Line 863: if ((read_status.IsEndOfFile() && live_blocks.empty())) {
Nit: there's an extra set of parens here.
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@865
PS4, Line 865: return Status::Aborted(Substitute("orphaned metadata file $0",common_path));
Nit: need an extra space between the comma and 'common_path'
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 08:28:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772
PS6, Line 772: con
> Won't this case be handled by the existing "delete dead containers at start
Hao and I discussed this on Slack. She pointed to the corresponding test case, where the LBM starts up with Status::Corruption. That's because the data file size was really 0, which meant that the dead blocks described the metadata file were treated as malformed in ProcessRecord(). I overlooked that, thinking that while the _number of bytes on disk_ was 0, the file size was something closer to FLAGS_log_container_max_size.
We could address that by deferring block consistency checks until after all DELETEs are processed, thus only checking live blocks. Later, the dead container cleanup code would remove the container.
But I don't think that needs to be addressed here, because the goal of this patch is to clean up the so-called "half present" containers, and in that case, the data file is missing outright. I'm not sure under what circumstances someone would end up with a zero-length data file; perhaps they accidentally truncated the data file? Or ran with fsync disabled and created+deleted one block?
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 25 Jan 2019 00:08:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12239
to look at the new patch set (#4).
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
This patch intends to delete the (orphaned) metadata file at
startup while the data file has gone missing and the metadata
file has no live blocks at the same time. In the previous patch
KUDU-2636, it has supported deleting the dead container which
includes data file and metadata file. So, there will be a time
window while deleting both of these two files in sequential,
and it will make a half-container in extreme cases, especially
in a crash or sent the tserver a kill -9. Indeed, Adar has
captured this type of case under 1000 runs of dense_node-itest.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 351 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/4
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 8:
Rebase the patch to pick up the python fix.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 21:57:51 +0000
Gerrit-HasComments: No
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 8: Verified+1 Code-Review+2
(1 comment)
Overriding Jenkins, the Python test failures have already been fixed upstream.
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772
PS6, Line 772: con
> As we repair when 'the data file exists + size == 0, the metadata file exis
Won't this case be handled by the existing "delete dead containers at startup" repair logic? That is, won't the container be deleted?
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 22:00:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12239
to look at the new patch set (#3).
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
This patch intends to delete the (orphaned) metadata file at
startup while the data file has gone missing and the metadata
file has no live blocks at the same time. In the previous patch
KUDU-2636, it has supported deleting the dead container which
includes data file and metadata file. So, there will be a time
window while deleting both of these two files in sequential,
and it will make a half-container in extreme cases, especially
in a crash or sent the tserver a kill -9. Indeed, Adar has
captured this type of case under 1000 runs of dense_node-itest.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 356 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/3
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager-test.cc@2055
PS5, Line 2055: ASSERT_OK(ReopenBlockManager(entity));
> Here you can use the FsReport to check that nothing was repaired. Same with
Done
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager.cc@869
PS5, Line 869: if (!read_status.IsEndOfFile() && !read_status.IsIncomplete()) {
> Should probably add a comment to explain this:
Done
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 5
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 01:33:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12239
to look at the new patch set (#6).
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
With KUDU-2636 fixed, we now support deleting dead containers at
runtime. Because this involves deleting a pair of files, there's
a time window in which it's possible for there to be just one file.
A well-timed crash or kill -9 can make this a permanent state that
fails the log block manager at startup. Indeed, a loop of
dense_node-itest failed 2/1000 times in this way.
This patch fixes the problem by detecting "orphaned" metadata files
at startup and deleting them. A metadata file is orphaned if it has
no corresponding data file and if it contains nothing but dead blocks.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 346 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/6
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
With KUDU-2636 fixed, we now support deleting dead containers at
runtime. Because this involves deleting a pair of files, there's
a time window in which it's possible for there to be just one file.
A well-timed crash or kill -9 can make this a permanent state that
fails the log block manager at startup. Indeed, a loop of
dense_node-itest failed 2/1000 times in this way.
This patch fixes the problem by detecting "orphaned" metadata files
at startup and deleting them. A metadata file is orphaned if it has
no corresponding data file and if it contains nothing but dead blocks.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Reviewed-on: http://gerrit.cloudera.org:8080/12239
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 349 insertions(+), 29 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 9
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1896
PS4, Line 1896: const auto CheckRepaired = [&] () {
> If you really want to test that the repair happened, you should look at the
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1987
PS4, Line 1987: // Create a metadata file with 1 byte which is <MIN.
: unique_ptr<WritableFile> metadata_file_writer;
: ASSERT_OK(env_->NewWritableFile(metadata_file_name, &metadata_file_writer));
: ASSERT_OK(metadata_file_writer->Append(Slice("a")));
: metadata_file_writer->Close();
> Since this pattern is repeated quite a few times, could you encapsulate it
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@2029
PS4, Line 2029: // Delete the only block.
: vector<BlockId> deleted;
: shared_ptr<BlockDeletionTransaction> transaction = bm_->NewDeletionTransaction();
: transaction->AddDeletedBlock(block_id);
: ASSERT_OK(transaction->CommitDeletedBlocks(&deleted));
: transaction.reset();
: // Wait for the actual hole punching to take place.
: for (const auto& data_dir : dd_manager_->data_dirs()) {
: data_dir->WaitOnClosures();
: }
> Likewise, encapsulate this in a lambda.
Done
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774: *-------------*------------*----------------*-------------------------*---------------------*/
> I don't think the order of which file is created/deleted matters because wi
Agreed:)
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801
PS1, Line 801: return Status::OK();
> If you look at all the filesystem calls made in this file, they're wrapped
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@762
PS4, Line 762: /* Note: the status here only represents the result of check.
> Could you reformat this to use multi-line C++ comments?
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@767
PS4, Line 767: EXIST && NO LIVE BLCOKS | EXIST && LIVE BLCOKS|
> BLOCKS
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@769
PS4, Line 769: (new case)
> Don't need this; if you really want to distinguish between the old and new
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@830
PS4, Line 830: report->incomplete_container_check->entries.emplace_back(common_path);
: return Status::Aborted(Substitute("orphaned empty metadata and data files $0", common_path));
> Nit: looks like these lines are indented too much.
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@839
PS4, Line 839: if (PREDICT_FALSE(s_data.IsNotFound())) {
: if (metadata_size >= pb_util::kPBContainerMinimumValidLength) {
> Can combine these and save one level of indentation.
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@863
PS4, Line 863: if ((read_status.IsEndOfFile() && live_blocks.empty())) {
> Nit: there's an extra set of parens here.
Done
http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@865
PS4, Line 865: return Status::Aborted(Substitute("orphaned metadata file $0",common_path));
> Nit: need an extra space between the comma and 'common_path'
Done
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 11:27:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12239
to look at the new patch set (#5).
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
With KUDU-2636 fixed, we now support deleting dead containers at
runtime. Because this involves deleting a pair of files, there's
a time window in which it's possible for there to be just one file.
A well-timed crash or kill -9 can make this a permanent state that
fails the log block manager at startup. Indeed, a loop of
dense_node-itest failed 2/1000 times in this way.
This patch fixes the problem by detecting "orphaned" metadata files
at startup and deleting them. A metadata file is orphaned if it has
no corresponding data file and if it contains nothing but dead blocks.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 336 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/5
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 5
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@768
PS6, Line 768: // |DATA\METADATA| NONE EXIST | EXIST && < MIN | EXIST && NO LIVE BLOCKS | EXIST && LIVE BLOCKS|
> I think it may be better to move this explanation to L546.
Done
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772
PS6, Line 772: OK
> Not sure why not auto repair this case as well?
I have no idea whether we need to repair this case, because this patch does not change the old logic, but only adds a new repaired case where an orphaned metadata file has no live blocks and no corresponding data file either.
So, do we need to fix it? @haohao @adar.
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@830
PS6, Line 830: metadata_size
> Since based on the current logic, metadata_size is 0 when the metada file d
Done
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@840
PS6, Line 840: quick check whether there is no live blocks
> nit: quickly check whether or not there is any live blocks.
Done
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@867
PS6, Line 867: orphaned metadata file
> nit: orphaned metadata file with no live blocks.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 08:15:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772
PS6, Line 772: con
> I have no idea whether we need to repair this case, because this patch does
As we repair when 'the data file exists + size == 0, the metadata file exists + size < min'. I don't see reasons why not treat the case 'the data file exists + size == 0, the metadata file exists + no live blocks' the same. But I am also fine with the current approach.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 7
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 21:57:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: LBM moves half-present containers out of the way
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: LBM moves half-present containers out of the way
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager-test.cc@1911
PS1, Line 1911: MetricRegistry new_registry;
: scoped_refptr<MetricEntity> new_entity =
: METRIC_ENTITY_server.Instantiate(&new_registry, "test");
> Can't you reuse 'entity' and 'registry' below?
Well, the 'registry' can be reused, but 'entity' can't. The reason is that there has been values in 'entity' already. And i think a new entity will be better.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@755
PS1, Line 755: // Check that both the metadata and data files exist and have valid lengths.
> It would be good to decompose this entire section into a separate function
hmm, maybe a new static function is appropriate.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774: // Handle a half-present container whose data file is missing. Open
> What about half-present containers whose metadata file is missing? I can't
In the destructor of 'LogBlockContainer', the data file is deleted before metadata file, so basically there is no case where the metadata file has gone missing.
Indeed, i came across a case where a data file went missing, but metadata not yet.
I think manual intervertion is necessary when the metadata file is missing in case, because it is not a common case.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@779
PS1, Line 779: set<BlockId> live_blocks;
> An unordered_set would be more efficient for this use case.
Done
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@780
PS1, Line 780: unique_ptr<RandomAccessFile> metadata_reader;
: RETURN_NOT_OK_CONTAINER_DISK_FAILURE(block_manager->env()->NewRandomAccessFile(
: metadata_path, &metadata_reader));
: ReadablePBContainerFile pb_reader(std::move(metadata_reader));
: RETURN_NOT_OK_CONTAINER_DISK_FAILURE(pb_reader.Open());
> Please consolidate this with the equivalent code in LogBlockManager::Proces
I did consider using the equivalent code from 'LogBlockManager::ProcessRecords'earlier. But there are two reasons why not:
1) the code in 'LogBlockManager::ProcessRecords' is much more complicated and some of them are needless for checking here;
2) more importantly, the 'LogBlockContainer::Open' is a static function and 'LogBlockManager::ProcessRecords' not;
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801
PS1, Line 801: if ((read_status.IsEndOfFile() && live_blocks.empty())) {
> What if read_status is not EndOfFile? Seems like that'll cause us to procee
Our initial goal was to cover scenarios when the data file has gone missing. In particular, an exception scenario occurs when the container is being deleted, but this does not include a operation system crash(which needs manual intervention). And only an operating system crash would cause half a record(a partial trailing record). So there is no need to notice that the read_status is not EndOfFile. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 21 Jan 2019 00:48:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12239
to look at the new patch set (#7).
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
With KUDU-2636 fixed, we now support deleting dead containers at
runtime. Because this involves deleting a pair of files, there's
a time window in which it's possible for there to be just one file.
A well-timed crash or kill -9 can make this a permanent state that
fails the log block manager at startup. Indeed, a loop of
dense_node-itest failed 2/1000 times in this way.
This patch fixes the problem by detecting "orphaned" metadata files
at startup and deleting them. A metadata file is orphaned if it has
no corresponding data file and if it contains nothing but dead blocks.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 349 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/7
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 7
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 8:
(1 comment)
LGTM. @Adar, I can merge it if you are also good with it.
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772
PS6, Line 772: con
> Won't this case be handled by the existing "delete dead containers at start
After more thought on this, I think manual intervention is better than auto repair since it is unclear why could end up in such case.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 25 Jan 2019 00:03:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager-test.cc@2055
PS5, Line 2055: ASSERT_OK(ReopenBlockManager(entity));
Here you can use the FsReport to check that nothing was repaired. Same with case 11.
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/5/src/kudu/fs/log_block_manager.cc@869
PS5, Line 869: if (!read_status.IsEndOfFile() && !read_status.IsIncomplete()) {
Should probably add a comment to explain this:
// If the read failed for some unexpected reason, propagate the error.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 5
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 17:49:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12239
to look at the new patch set (#2).
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
[fs]: delete the (orphaned) metadata file when repairing
This patch intends to delete the (orphaned) metadata file while
the data file has gone missing and the metadata file has no live
blocks at the same time.
In the previous patch KUDU-2636, it has supported deleting the
dead container which includes data file and metadata file. So there
will be a time window between deleting these both two files, and
in extreme cases(especially in a crash or send the tserver a kill -9)
it will make a half-container. @adar has captured this type of case
under 1000 runs of dense_node-itest.
Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 354 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12239/2
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: LBM moves half-present containers out of the way
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: LBM moves half-present containers out of the way
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12239/1//COMMIT_MSG
Commit Message:
PS1:
The commit message doesn't accurately describe the change. We now look for cases where the data file has gone missing, test whether the metadata file has no live blocks, and if that's true, we'll delete the (orphaned) metadata file when repairing.
It'd also be good to explain _why_ this change is necessary, and how it ties into your previous work.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Jan 2019 19:37:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: LBM moves half-present containers out of the way
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: LBM moves half-present containers out of the way
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager-test.cc@1911
PS1, Line 1911: MetricRegistry new_registry;
: scoped_refptr<MetricEntity> new_entity =
: METRIC_ENTITY_server.Instantiate(&new_registry, "test");
Can't you reuse 'entity' and 'registry' below?
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@755
PS1, Line 755: // Check that both the metadata and data files exist and have valid lengths.
It would be good to decompose this entire section into a separate function whose goal is to sanity check the container files.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774: // Handle a half-present container whose data file is missing. Open
What about half-present containers whose metadata file is missing? I can't think of a safe way to handle this:
1. Check if the data file has 0 length. Not useful because a full container will have a 10G length.
2. Check if the data occupies 0 bytes on disk. Not reliable because we may have had multiple concurrent deletions at the time of the crash, some of which were going to punch holes in the data file (but didn't).
In your runs of dense_node-itest, have you seen cases where the metadata file went missing? Maybe we should rename these orphaned data files? Would probably have to beef up FsReport and Repair() to handle that case, since we wouldn't want to do it in read-only filesystems.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@779
PS1, Line 779: set<BlockId> live_blocks;
An unordered_set would be more efficient for this use case.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@780
PS1, Line 780: unique_ptr<RandomAccessFile> metadata_reader;
: RETURN_NOT_OK_CONTAINER_DISK_FAILURE(block_manager->env()->NewRandomAccessFile(
: metadata_path, &metadata_reader));
: ReadablePBContainerFile pb_reader(std::move(metadata_reader));
: RETURN_NOT_OK_CONTAINER_DISK_FAILURE(pb_reader.Open());
Please consolidate this with the equivalent code in LogBlockManager::ProcessRecords.
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801
PS1, Line 801: if ((read_status.IsEndOfFile() && live_blocks.empty())) {
What if read_status is not EndOfFile? Seems like that'll cause us to proceed as if the container is fine on L817.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Jan 2019 19:35:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Removed reviewer Kudu Jenkins.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12239/4//COMMIT_MSG@9
PS4, Line 9: This patch intends to delete the (orphaned) metadata file at
: startup while the data file has gone missing and the metadata
: file has no live blocks at the same time. In the previous patch
: KUDU-2636, it has supported deleting the dead container which
: includes data file and metadata file. So, there will be a time
: window while deleting both of these two files in sequential,
: and it will make a half-container in extreme cases, especially
: in a crash or sent the tserver a kill -9. Indeed, Adar has
: captured this type of case under 1000 runs of dense_node-itest.
> Thanks for the added detail. Could you reword this a bit to add more clarit
Done
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 23 Jan 2019 11:29:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: LBM moves half-present containers out of the way
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: LBM moves half-present containers out of the way
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774
PS1, Line 774: // Handle a half-present container whose data file is missing. Open
> In the destructor of 'LogBlockContainer', the data file is deleted before m
In addition, the metadata file is created before data file in 'LogBlockContainer::Create'.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 21 Jan 2019 06:01:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 01:52:31 +0000
Gerrit-HasComments: No
[kudu-CR] [fs]: delete the (orphaned) metadata file when repairing
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@768
PS6, Line 768: // |DATA\METADATA| NONE EXIST | EXIST && < MIN | EXIST && NO LIVE BLOCKS | EXIST && LIVE BLOCKS|
I think it may be better to move this explanation to L546.
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@772
PS6, Line 772: OK
Not sure why not auto repair this case as well?
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@830
PS6, Line 830: metadata_size
Since based on the current logic, metadata_size is 0 when the metada file does not exist, this condition covers not only empty metadata file + zero data size, but also for missing metadata file + zero data size? If so, could you please update the comment above? Also, it would be more clear to use 's_meta.IsNotFound()' for checking missing metadata file.
Same for 'Same for data_size==0'.
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@840
PS6, Line 840: quick check whether there is no live blocks
nit: quickly check whether or not there is any live blocks.
http://gerrit.cloudera.org:8080/#/c/12239/6/src/kudu/fs/log_block_manager.cc@867
PS6, Line 867: orphaned metadata file
nit: orphaned metadata file with no live blocks.
--
To view, visit http://gerrit.cloudera.org:8080/12239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab
Gerrit-Change-Number: 12239
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 06:06:20 +0000
Gerrit-HasComments: Yes