Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5240: Allow config of number of disk I/O threads per disk type ......................................................................
Patch Set 8: (7 comments) looks good, very close now, thanks http://gerrit.cloudera.org:8080/#/c/7232/8//COMMIT_MSG Commit Message: PS8, Line 24: TODO (Request comments on this additional change): ? remove PS8, Line 29: used , by the actual code. They were actually being used by the test code. see disk-io-mgr-test: DiskIoMgr::ScanRange* InitRange(int num_buffers, const char* file_path, int offset, int len, int disk_id, int64_t mtime, void* meta_data = NULL, bool is_cached = false) the disk_id is explicitly set. PS8, Line 29: As a part of this effort, existing test cases in : disk-io-mgr-test were identified that exploited this bug and will be : tracked in IMPALA-5574. This is overstating the problem. We need to call out IMPALA-5574, it will just distract from the core issue being fixed here. Please just say: The tests have been updated to use the new DiskIoMgr test constructor, but retain the same behavior as before. http://gerrit.cloudera.org:8080/#/c/7232/8/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS8, Line 48: 1 If we change the constants later, we'll probably forget to update this. You can just define the static const ints above this and use Substitute to print them in to the msg. Line 54: "8 threads per solid state disk"); same http://gerrit.cloudera.org:8080/#/c/7232/7/be/src/util/disk-info.h File be/src/util/disk-info.h: PS7, Line 59: static bool is_rotational(int disk_id) { : DCHECK_GE(disk_id, 0) > I believe having the DCHECK back makes it consistent with the semantics of I think that these new semantics make sense, but if you really wanted to handle this differently, a DCHECK isn't the right way to address the problem that the API allows you to give it bad values without a way to report them: i.e. a better API is something like enum DiskType {...}; Status GetDiskType(int disk_id, DiskType* out); Then this can return an error (e.g. disk doesn't exist, or maybe we just can't tell). Avoiding bool would also be more clear. Anyway, we won't actually do this now. http://gerrit.cloudera.org:8080/#/c/7232/8/be/src/util/disk-info.h File be/src/util/disk-info.h: Line 61: if(disk_id >= disks_.size()) return false; space after if -- To view, visit http://gerrit.cloudera.org:8080/7232 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I094aff3747104104fe0465d73dcdbef5d9892f7c Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
