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 3: (21 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 http://gerrit.cloudera.org:8080/#/c/10415/3//COMMIT_MSG@14 PS3, Line 14: upated updated http://gerrit.cloudera.org:8080/#/c/10415/3//COMMIT_MSG@18 PS3, Line 18: nit: extra whitespace 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 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_) 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_ (same for cpu_sys_) 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 fragment instance. 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@218 PS3, Line 218: resource_utilization->backend_scanned_bytes = resource_utilization_.backend_scanned_bytes; long line 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 AggregateBackendStats updated in this method? 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 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 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 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 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 under a struct named "BackendResourceUtilization" http://gerrit.cloudera.org:8080/#/c/10415/3/be/src/runtime/coordinator.h@179 PS3, Line 179: cpu_user nit: cpu_user_time_ (same below) 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 metrics across all backends 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 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? If the objective of doing this was to make the names of the variables shorter for readability, perhaps we can shorten "backend_resource_utilization" to just "resource_usage" http://gerrit.cloudera.org:8080/#/c/10415/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10415/1/be/src/service/impala-server.cc@1015 PS1, Line 1015: _QUERY << "Qu > maybe have a named constant for this or a default constructor specifically can you address this too? 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@1909 PS3, Line 1909: // 4. Check and cancel queries with Cpu and scan bytes constraints if limit is exceeded nit:long line 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 expiration event. If the objective is to just check for resource limits every-time this thread comes up, perhaps we should use a separate list than "queries_by_timestamp_" since these checks are not dependent on the "query timestamp" -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 29 May 2018 18:27:26 +0000 Gerrit-HasComments: Yes
