Adar Dembo has posted comments on this change. Change subject: block_manager: various changes to disk space reservation checking ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4831/1/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 195: } > the FALLTHROUGH_INTENDED macro is probably nice here Done Line 201: LOG(ERROR) << Substitute( > WARNING seems more appropriate Okay, though this was just a cut/paste from the LBM code. PS1, Line 211: if (metrics_ && is_full_ != is_full_new) { : metrics_->data_dirs_full->IncrementBy(is_full_new ? 1 : -1); : } > shouldn't this bit be under a lock? Oops, yes it should. PS1, Line 387: // Update fullness information for data dirs that were last determined to be : // full some time ago. : > why not just do this for the data dir you're about to return? seems strange Good point. This was largely inspired by the existing LBM approach with FullDiskCache, where a call to CreateBlock() expired all entries in the cache. http://gerrit.cloudera.org:8080/#/c/4831/1/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS1, Line 86: CheckIsFul > I think a name like 'RefreshFullness' or 'RefreshIsFull' might be better. " OK, will rename. Line 141: Status GetNextDataDir(DataDir** dir); > add a doc about when a bad Status is returned. Does it guarantee for an OK Will doc. Yes, that is guaranteed. -- To view, visit http://gerrit.cloudera.org:8080/4831 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica2bb710a246ef09890554d1d6ff6da528145a6e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
