Todd Lipcon 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 Line 201: LOG(ERROR) << Substitute( WARNING seems more appropriate 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? 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 to be O(n) here 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. "CheckIsFull' sounds like a method that verifies that the disk is full and returns a bad status if it's not full. Line 141: Status GetNextDataDir(DataDir** dir); add a doc about when a bad Status is returned. Does it guarantee for an OK status that *dir is non-NULL? -- 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: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
