[kudu-CR] [fs manager] Disallow write operations on read only fs handles

["Adar Dembo (Code Review)" <ge...@cloudera.org>]

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 <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No