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 Mon, 26 Jun 2017 18:02:54 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 4:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/7232/4//COMMIT_MSG
Commit Message:

Line 34: 
> Comment that we are changing the tests to explicitly set num_disks=1 for no
Done


http://gerrit.cloudera.org:8080/#/c/7232/4/be/src/runtime/disk-io-mgr-stress-test.cc
File be/src/runtime/disk-io-mgr-stress-test.cc:

Line 31: const int NUM_DISKS = 1;
> comment why we are setting this to 1 for now, i.e. that the test code isn't
I have added a TODO here as well as per your other comment in disk-io-mgr-test


http://gerrit.cloudera.org:8080/#/c/7232/3/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

Line 1142:   const int num_io_threads_per_rotational_or_ssd = 2;
> const
Done


PS3, Line 1165: 
> weird indentation, just put the operator on the previous line and use 4 spa
Done


http://gerrit.cloudera.org:8080/#/c/7232/4/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

Line 212:   const int num_disks = 1;
> leave a TODO with the JIRA where we'll change this, same for the other case
Done


http://gerrit.cloudera.org:8080/#/c/7232/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:

PS4, Line 87: // Rotational disks should have 1 thread per disk to minimize seeks.  Non-rotational
            : // don't have this penalty and benefit from multiple concurrent IO requests.
            : static const int THREADS_PER_ROTATIONAL_DISK = 1;
            : static const int THREADS_PER_FLASH_DISK = 8;
> I don't think there's any reason to keep these separately and then check if
We need the default value of the flags to be zero in order to figure out if they were set
or not. That is needed to follow the priority order of :
1. FLAG_num_io_threads_per_rotational_disk
2. num_threads_per_disk
3. THREADS_PER_ROTATIONAL_DISK


PS4, Line 263:     num_io_threads_per_rotational_disk_(FLAGS_num_io_threads_per_rotational_disk),
             :     num_io_threads_per_solid_state_disk_(FLAGS_num_io_threads_per_solid_state_disk),
> I think we can configure num_io_threads_per_rotational_disk_ and num_io_thr
Done


Line 278:         " disks";
> To be consistent we should also warn if the value is negative
Done


PS4, Line 367:     } else if (num_io_threads_per_rotational_disk_ != 0 && DiskInfo::is_rotational(i))
{
             :       num_threads_per_disk = num_io_threads_per_rotational_disk_;
             :     } else if (num_io_threads_per_solid_state_disk_ != 0 && !DiskInfo::is_rotational(i))
{
             :       num_threads_per_disk = num_io_threads_per_solid_state_disk_;
             :     } else if (FLAGS_num_threads_per_disk != 0) {
             :       num_threads_per_disk = FLAGS_num_threads_per_disk;
             :     } else if (DiskInfo::is_rotational(i)) {
             :       num_threads_per_disk = THREADS_PER_ROTATIONAL_DISK;
             :     } else {
             :       num_threads_per_disk = THREADS_PER_FLASH_DISK;
             :     }
> After my suggestion in the constructor this becomes greatly simplified.
Done


http://gerrit.cloudera.org:8080/#/c/7232/3/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:

PS3, Line 631: ///
> comment this is for testing only
Done


PS3, Line 837: o the disk.
> the naming of this should probably be num_io_threads_per_disk_ to be consis
Went ahead with your latter suggestion and removed it altogether


http://gerrit.cloudera.org:8080/#/c/7232/3/be/src/util/thread.cc
File be/src/util/thread.cc:

PS3, Line 335: const
> nit: space after const
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: 4
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