You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/08/23 17:23:03 UTC

[kudu-CR] error manager: rename error types

Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11303


Change subject: error_manager: rename error types
......................................................................

error_manager: rename error types

The previous ErrorHandlerType enum names weren't very descriptive, and
reflected the expected error handling (e.g. fail a disk, fail a tablet),
rather than the error itself. In preparation of adding a new error
ErrorHandlerType for CFile checksum errors, this patch attempts to
clarify usage of the FsErrorManager.

While I was in the area, I updated the error manager to use a map to
store its error-handling callbacks, as I intend on adding another error
type.

Change-Id: I377904ee51bb0f7b0f1104d7a4723f74d52e6e3f
---
M src/kudu/fs/error_manager-test.cc
M src/kudu/fs/error_manager.cc
M src/kudu/fs/error_manager.h
M src/kudu/fs/file_block_manager.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/tserver/tablet_server.cc
8 files changed, 88 insertions(+), 101 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I377904ee51bb0f7b0f1104d7a4723f74d52e6e3f
Gerrit-Change-Number: 11303
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>

[kudu-CR] error manager: rename error types

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11303 )

Change subject: error_manager: rename error types
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11303/4/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

http://gerrit.cloudera.org:8080/#/c/11303/4/src/kudu/gutil/map-util.h@457
PS4, Line 457:   typedef typename Collection::mapped_type mapped_type;
             :   auto it = collection->find(key);
             :   if (it == collection->end()) {
             :     collection->emplace(key, std::forward<mapped_type>(value));
             :     return true;
             :   }
             :   it->second = std::forward<mapped_type>(value);
             :   return false;
> Ah, yeah seems indeed we can't presume:
I spent some time on this but couldn't make it work.

For one, forwarding args to emplace() seems to destroy the args, even if emplace() fails. You observed this as well.
For two, doing find()/emplace() is tricky because I couldn't get the move assignment to work in the event that find() returns an existing element.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I377904ee51bb0f7b0f1104d7a4723f74d52e6e3f
Gerrit-Change-Number: 11303
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Aug 2018 18:44:41 +0000
Gerrit-HasComments: Yes