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