Bikramjeet Vig 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: (11 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 mentioned in the "testing" part but would still prefer that its mentioned explicitly in the description. 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 http://gerrit.cloudera.org:8080/#/c/10415/9//COMMIT_MSG@14 PS9, Line 14: Query profile is updated to include query wide and per backend metrics for Cpu nit:long line also include an example 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::GetResourceUtilization(). Seems like we are only concerned with the aggregate level ResourceUtilization if ComputeQuerySummary() is called (only called at a terminal state) or if ComputeQueryResourceUtilization() is called (which only happens if there is a resource limit on the query). This deferred calculation will help get rid of BackendState::lock_ required in this method and the logic around delta updates of ResourceUtilization. Moreover, queries that dont have a resource limit wont have to pay for this locking/updating cost everytime an update is recieved. NOTE: if we envision resource management to always set a default limit on resources then we can just stick with the current implementation as having resources limits would be the norm rather than the exception. http://gerrit.cloudera.org:8080/#/c/10415/9/be/src/runtime/coordinator-backend-state.cc@600 PS9, Line 600: value->AddMember("peak_mem_consumption", resource_utilization_.peak_mem_consumption, : document->GetAllocator()); maybe add other members of resource_utilization_ as well here 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 query level, maybe clarify what we mean here. 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 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-ok status we call ExpireQuery(). This way its sounds intuitive what the name CheckResourceLimits() is doing and its consistent with the how the rest of the method behaves. Dont have a strong preference so feel free to ignore this comment. 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 in seconds 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 options. 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 defined in the thrift file -- 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 <mmokh...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 14 Jul 2018 02:05:11 +0000 Gerrit-HasComments: Yes
