Adar Dembo has posted comments on this change. Change subject: [fs_manager] Disallow write operations on read_only fs handles ......................................................................
Patch Set 1: Code-Review-1 I remember our discussion that led to this patch, but in retrospect it looks like I misunderstood the conversation. I thought you had found some FsManager functions that 1) make destructive changes, and 2) weren't enforcing that read_only_ was set prior to making those changes. It made sense to me that such functions should enforce it, and I thought that's what you were working on. I'm not a fan of the changes in this patch though. The intent is good; it makes sense to avoid making changes if we're in read-only mode. However, to accomplish that, we're consulting FsManager state, which is a bit of an encapsulation leak. There's also no guarantee that future functions that make changes will consult FsManager state. If you want to push forward with this, here's a cleaner alternative: encapsulate that actual destructive operations within the FsManager. For example, convert this: CHECK(fs_manager_.read_only()); env_->DeleteFromDisk(...); Into this: fs_manager_.DeleteFromDisk(...); And then check read_only inside FsManager::DeleteFromDisk() (a new function). That way, read_only() isn't leaked outside of FsManager, and any new code that wants to do the same thing can make a single call and get enforcement for free. Relatedly, I think FsManager::read_only() shouldn't exist (or should be private), but it looks like there's a legitimate use case inside TabletMetadata::LoadFromSuperBlock() right now. IMHO TabletMetadata should maintain its own read-only state (just like the block managers do) and consult that to decide whether to delete orphaned blocks. -- To view, visit http://gerrit.cloudera.org:8080/4891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8425ce3e739d0d37869edc879fc16dc48c89eb7e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
