Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10415 )
Change subject: IMPALA-6034: Add CPU and scanned bytes limits per query ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/10415/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10415/9//COMMIT_MSG@8 PS9, Line 8: > maybe mention the query_option names added in the patch. I know its mention Done http://gerrit.cloudera.org:8080/#/c/10415/9//COMMIT_MSG@10 PS9, Line 10: scanned bytes, limits are enforced via query options and applied to the entire > nit: long line Done http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/runtime/coordinator-backend-state.cc@500 PS9, Line 500: : // Update resource utilization and apply delta. : ResourceUtilization prev_utilization = resource_utilization_; : RuntimeProfile::Counter* user_time = profile_->GetCounter("TotalThreadsUserTime"); : if (user_time != nullptr) resource_utilization_.cpu_user_ns = user_time->value(); : : RuntimeProfile::Counter* system_time = profile_->GetCounter("TotalThreadsSysTime"); : if (system_time != nullptr) resource_utilization_.cpu_sys_ns = system_time->value(); : : int total_bytes = 0; : for (RuntimeProfile::Counter* c: bytes_read_counters_) total_bytes += c->value(); : resource_utilization_.bytes_read = total_bytes; : : RuntimeProfile::Counter* peak_mem = : profile_->GetCounter(FragmentInstanceState::PER_HOST_PEAK_MEM_COUNTER); : if (peak_mem != nullptr) resource_utilization_.peak_mem_consumption = peak_mem->value(); : aggregate_utilization->Merge(resource_utilization_.Delta(prev_utilization)); > I feel we should just defer this calculation to BackendState::GetResourceUt Removed the statefulness, agree that computing it on demand makes more sense overall. http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/runtime/coordinator.h@171 PS9, Line 171: for this query > nit: since this struct can hold info for either finstance, backend and quer Done http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/impala-server.cc@2026 PS9, Line 2026: crs > love the variable name Done http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/impala-server.cc@2036 PS9, Line 2036: CheckResourceLimits > nit: how about we return a status from CheckResourceLimits(), and on a non- Done http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/impala-server.cc@2125 PS9, Line 2125: TIME_NS > we can just use seconds here instead as the cpu_limit_s can only we defined Done http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/query-options-test.cc@146 PS9, Line 146: > nit: not a huge deal but we can probably get this in line with the first 5 I just ran it through clang-format :). I generally dislike this kind of alignment because it forces people coming along later to either write inconsistent code or make a bunch of spurious whitespace changes to keep everything aligned. http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/service/query-options.h@136 PS9, Line 136: QUERY_OPT_FN(scan_bytes_limit, SCAN_BYTES_LIMIT,\ : TQueryOptionLevel::ADVANCED)\ : QUERY_OPT_FN(cpu_limit_s, CPU_LIMIT_S,\ : TQueryOptionLevel::ADVANCED)\ > can you move these to the end, just to be consistent with how they are defi Done -- To view, visit http://gerrit.cloudera.org:8080/10415 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c6015e21da684bb9f33e236d71309dd4c178a20 Gerrit-Change-Number: 10415 Gerrit-PatchSet: 9 Gerrit-Owner: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 17 Jul 2018 00:43:50 +0000 Gerrit-HasComments: Yes
