You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/08/31 01:55:40 UTC

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata 

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/4178

to review the following change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................

KUDU-668. log block manager should tolerate empty metadata

This solves a commonly seen issue after a server crashes due to "Too
many open files". In many cases, this type of crash would leave an empty
metadata file and a missing data file.

This isn't a full fix for KUDU-668, but handles the above common case by
just checking the size and existence of these files. If the metadata is
too small to be valid, and the data is empty, then we just remove the
container and continue with startup.

Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
4 files changed, 70 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/4178/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4178/1/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

PS1, Line 975:   s = this->ReopenBlockManager(scoped_refptr<MetricEntity>(),
             :                                shared_ptr<MemTracker>(),
             :                                { GetTestDataDirectory() },
             :                                false);
             :   ASSERT_OK(s);
> Nit: just ASSERT_OK(this->ReopenBlockManager(...)) ?
Done


http://gerrit.cloudera.org:8080/#/c/4178/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 430:                    << " are both missing or empty. Skipping container.";
> Maybe "Deleting container"? Should note that we'll actually delete the file
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3239/

-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


Patch Set 1:

(2 comments)

Yeah, we can be more forgiving in some cases (i.e. data_size >= but metadata_size == minimum). But it's not a huge deal.

http://gerrit.cloudera.org:8080/#/c/4178/1/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

PS1, Line 975:   s = this->ReopenBlockManager(scoped_refptr<MetricEntity>(),
             :                                shared_ptr<MemTracker>(),
             :                                { GetTestDataDirectory() },
             :                                false);
             :   ASSERT_OK(s);
Nit: just ASSERT_OK(this->ReopenBlockManager(...)) ?


http://gerrit.cloudera.org:8080/#/c/4178/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 430:                    << " are both missing or empty. Skipping container.";
Maybe "Deleting container"? Should note that we'll actually delete the files.


-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


KUDU-668. log block manager should tolerate empty metadata

This solves a commonly seen issue after a server crashes due to "Too
many open files". In many cases, this type of crash would leave an empty
metadata file and a missing data file.

This isn't a full fix for KUDU-668, but handles the above common case by
just checking the size and existence of these files. If the metadata is
too small to be valid, and the data is empty, then we just remove the
container and continue with startup.

Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Reviewed-on: http://gerrit.cloudera.org:8080/4178
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
4 files changed, 69 insertions(+), 12 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3157/

-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................


Patch Set 1:

fwiw I also just tried this patch on a test cluster which had hit this issue, and some servers that previously couldn't start now started OK

-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-668. log block manager should tolerate empty metadata

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4178

to look at the new patch set (#2).

Change subject: KUDU-668. log block manager should tolerate empty metadata
......................................................................

KUDU-668. log block manager should tolerate empty metadata

This solves a commonly seen issue after a server crashes due to "Too
many open files". In many cases, this type of crash would leave an empty
metadata file and a missing data file.

This isn't a full fix for KUDU-668, but handles the above common case by
just checking the size and existence of these files. If the metadata is
too small to be valid, and the data is empty, then we just remove the
container and continue with startup.

Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
4 files changed, 69 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/4178/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4178
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4fd9d0f510835d100d35efaf541d34829118f56
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>