impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5240: Allow config of number of disk I/O threads per disk type
Date Fri, 30 Jun 2017 17:29:30 GMT
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 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, the only future
work is to consider changing the BE tests to actually have files on different disks, but that's
basically unrelated to your change.


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


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_disk isn't set.


Line 51: DEFINE_int32(num_io_threads_per_solid_state_disk, 0, "Number of I/O threads per solid"
same


Line 262: static inline int GetFirstPositiveVal(const int first_val, const int second_val,
1 line comment why this exists


PS7, Line 273: FLASH
inconsistent FLASH vs ssd naming


PS7, Line 284: &&
this should be ||


PS7, Line 375: e
add space


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 rotational disk,
is false otherwise.


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 the number of
disks they can use num_disks().


PS7, Line 64:   
nit remove whitespace


PS7, Line 80:  
remove trailing whitespace


-- 
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 <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message