Dinesh Bhat has posted comments on this change. Change subject: [fs_manager] Disallow write operations on read_only fs handles ......................................................................
Patch Set 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. Thanks for reviews Adar, yeah I agree with what you said above. I will make the changes along the lines suggested above. One follow-up question though: Even if we provide fs_manager_.DeleteXYZ() routines, how can we prevent the caller from directly doing fs_manager->env()->deleteFile() ? perhaps we could keep the Env as private inside FsManager and provide public access to env() routines from FsManager ? Like FsManager::DeleteFile, FsManager::CreateDir() etc. PS: Above follow-up question is somewhat along the lines I was discussing with you on slack about enforcing these restrictions at Env/PosixEnv assuming we have enough context about files opened etc. There we had converged that we wanted to keep those classes as stateless and as close to system-calls as possible and I concur with that. -- 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: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
