Mostafa Mokhtar 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 3: (22 comments) http://gerrit.cloudera.org:8080/#/c/10415/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10415/3//COMMIT_MSG@10 PS3, Line 10: apply > nit: applied Done http://gerrit.cloudera.org:8080/#/c/10415/3//COMMIT_MSG@14 PS3, Line 14: upated > updated Done http://gerrit.cloudera.org:8080/#/c/10415/3//COMMIT_MSG@18 PS3, Line 18: > nit: extra whitespace Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.h@113 PS3, Line 113: fragments > nit: fragment Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.h@200 PS3, Line 200: /// used by Coordinator::BackendState::AggregateBackendStats > maybe mention units. (same for cpu_sys_) Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.h@201 PS3, Line 201: cpu_user_ > nit: how about cpu_user_time_ Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.h@207 PS3, Line 207: BYTES_READ_COUNTERs in profile_ > nit: Collection of BYTES_READ_COUNTERs of all the scan nodes in this fragme Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.cc@213 PS3, Line 213: void Coordinator::BackendState::GetBackendResourceUtilization( > We could just return BackendResourceUtilization by value and achieve the sa Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.cc@341 PS3, Line 341: AggregateBackendStats > do we need this call here? are any of the instance stats used in AggregateB Coordinator::ComputeQuerySummary calls Coordinator::BackendState::UpdateExecStats before the final metrics are written to the query profile, otherwise stale info will be written. http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.cc@527 PS3, Line 527: RuntimeProfile::Counter* profile_user_time_counter = profile_->GetCounter("TotalThreadsUserTime"); > nit: long line Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.cc@533 PS3, Line 533: RuntimeProfile::Counter* profile_system_time_counter = profile_->GetCounter("TotalThreadsSysTime"); > nit: long line Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator-backend-state.cc@638 PS3, Line 638: "peak_mem_consumption", resource_utilization_.peak_consumption, document->GetAllocator()); > nit:long line Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.h@173 PS3, Line 173: peak_consumption > nit: how about peak_mem_consumption Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.h@176 PS3, Line 176: backend_ > nit: we can probably get rid of this prefix now that all these counters are Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.h@185 PS3, Line 185: Aggregate CPU and bytes read metrics > update comment: Aggregate CPU, scanned bytes and peak memory consumption me Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.cc@725 PS3, Line 725: info > unused variable Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.cc@732 PS3, Line 732: peak_memory = backend_resource_utilization.peak_consumption; : backend_user_cpu = backend_resource_utilization.backend_cpu_user; : backend_sys_cpu = backend_resource_utilization.backend_cpu_sys; : backend_scanned_bytes = backend_resource_utilization.backend_scanned_bytes; > nit: can we directly use the values in the struct? Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/service/impala-server.cc@1011 PS3, Line 1011: queries_by_timestamp_.emplace(ExpirationEvent{ > We only need to queue one expiration event if both max_cpu_time_s and max_s Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/service/impala-server.cc@1909 PS3, Line 1909: // 4. Check and cancel queries with Cpu and scan bytes constraints if limit is exceeded > nit:long line Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/service/impala-server.cc@1934 PS3, Line 1934: query_resouce_usage > type:query_resource_usage Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/service/impala-server.cc@1972 PS3, Line 1972: } > looks like we are not updating the deadline for RESOURCE_LIMIT type of expi Done http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/service/impala-server.cc@1972 PS3, Line 1972: } > Ah, good catch. Yeah I think we should do something similar to the query ti 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: 3 Gerrit-Owner: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Jun 2018 21:05:59 +0000 Gerrit-HasComments: Yes
