impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (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 18:57:16 GMT
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 <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