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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
