Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-5240: Allow config of number of disk I/O threads per disk type ......................................................................
Patch Set 7: (12 comments) http://gerrit.cloudera.org:8080/#/c/7232/7//COMMIT_MSG Commit Message: PS7, Line 24: TODO (Request comments on this additional change): > remove TODO line and update the message below to indicate this all works, t Done http://gerrit.cloudera.org:8080/#/c/7232/7/be/src/runtime/disk-io-mgr-stress.cc File be/src/runtime/disk-io-mgr-stress.cc: Line 75: MIN_READ_BUFFER_SIZE, MAX_READ_BUFFER_SIZE)); > nit 4 spaces tab Done http://gerrit.cloudera.org:8080/#/c/7232/7/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: Line 48: " disk. Has priority over num_threads_per_disk."); > It'd be good to indicate what this defaults to if this and num_threads_per_ Done Line 51: DEFINE_int32(num_io_threads_per_solid_state_disk, 0, "Number of I/O threads per solid" > same Done Line 262: static inline int GetFirstPositiveVal(const int first_val, const int second_val, > 1 line comment why this exists Done PS7, Line 273: FLASH > inconsistent FLASH vs ssd naming Done PS7, Line 284: && > this should be || Done PS7, Line 375: e > add space Done http://gerrit.cloudera.org:8080/#/c/7232/7/be/src/util/disk-info.h File be/src/util/disk-info.h: Line 56: > add a comment that this returns true if the disk w/ disk_id exists and is a Done PS7, Line 59: //TODO: temporarily removed DCHECK due to an issue tracked in IMPALA-5574, put it back : // after its resolved > remove, I don't think this needs to be temporary. If someone wants to check I believe having the DCHECK back makes it consistent with the semantics of this method, like if is_rotational is false, it should mean its another type of disk, but calling it with a non-existent disk_id should be illegal and should be caught by the DCHECK (like it also is in device_name()) PS7, Line 64: > nit remove whitespace Done PS7, Line 80: > remove trailing whitespace Done -- 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: 7 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
