Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8395 )
Change subject: error_manager: synchronize/serialize handling ...................................................................... Patch Set 5: > Patch Set 5: > > (2 comments) Thanks for the proposal! I've been thinking through this a bit, about potential issues/benefits. I agree it would be fewer calls to error handling. My first impression is that it treats all files the same, when maybe we shouldn't. That would assume that disk failure handling should be the same for all kinds of files, which might not be true (e.g. how would we handle EIOs in cmeta/tmeta vs for a block?). I suppose that could be sorted out in the callback (matching the file to the sort of handling or somesuch). A large, maybe subtle change that I think you're proposing is that the error manager would be owned by the Env. This feels weird, but I could see it. Wiring could be done by the tablet_server as it is now, going through `fs_manager()->env()->RegisterCallback()` or somesuch. In this case, though, I think it'd be weird to handle any errors outside of the Env layer (e.g. the aforementioned tablet data corruptions, checksums, etc.), since most things only know about the FsManager, not the Env (again, could be remedied with the `fs_manager()->env()`). Of course, _all_ of the error-handling plumbing would move from the block manager to the Env, which is a pretty significant change. This also doesn't address the issue in the commit message, where errors may be indirectly caused by disk failures. It's a "tablet" error, so I suppose it will have to reach into the Env and trigger error-handling explicitly. That sort of feels weird too, since now we're conflating the layers that we're handling these errors (not a dealbreaker, and not not saying that the current impl is free of this problem, but worth bringing up). Overall the (biased!) impression I get is that it seems a bit less flexible, since most things are ignorant of the Env, but a lot of things know about the FsManager (easily worked around though). I'll have to think about it a bit more. There may also some more intricacies to think about regarding the lifecycle of the block manager, dir manager, etc. -- To view, visit http://gerrit.cloudera.org:8080/8395 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie61c408a0b4424f933f40a31147568c2f906be0e Gerrit-Change-Number: 8395 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 14 Nov 2017 04:11:50 +0000 Gerrit-HasComments: No
