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, 23 Jun 2017 00:38:54 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 4:

(7 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 now because the test
code isn't yet aware of how many disks are actually on the test system and the test code also
isn't updated to actually use multiple disks.
We should also have the JIRA to reference.


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 yet aware of how
many disks are actually on the test system and the test code also isn't updated to actually
use multiple disks.

We should also have the JIRA to reference.


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 cases below.


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 FLAGS_x are non-zero.
Let's just make these the default value of those flags, then we can simplify the logic in
Init()


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_threads_per_solid_state_disk_
based on the flags once, rather than having to have the logic in Init() multiple times.

E.g.

if (FLAGS_num_io_threads_per_rotational_disk > 0) {
  num_io_threads_per_rotational_disk_ = FLAGS_num_io_threads_per_rotational_disk;
} else if (FLAGS_num_io_threads_per_disk > 0) {
  num_io_threads_per_rotational_disk_ = FLAGS_num_io_threads_per_disk;
} else {
  num_io_threads_per_rotational_disk_ = DEFAULT_NUM_PER_ROT_DISK;
}

same for ssd.


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


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.


-- 
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