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 3: (5 comments) 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 same thing (copying all the members into the caller's stack allocation) I.e. BackendResourceUtilization Coordinator::BackendState::GetBackendResourceUtilization() { lock_guard<mutex> l(lock_); return resource_utilization_; } 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@815 PS3, Line 815: query_resource_utilization->peak_consumption = max( If we add more counters like this at some point we should come up with some shared logic for doing the aggregation instead of stamping it out. That seems like overkill for now though... 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_scan_bytes are set. I.e. it should be something like: if (max_cpu_time_s > 0 || max_scan_bytes > 0) { if (max_cpu_time_s > 0) { VLOG_QUERY << "Query " << PrintId(query_id) << " has cpu limit of " << PrettyPrinter::Print(max_cpu_time_s, TUnit::TIME_S); } if (max_scan_bytes > 0) { VLOG_QUERY << "Query " << PrintId(query_id) << " has scan bytes limit of " << PrettyPrinter::Print(max_scan_bytes, TUnit::BYTES); } queries_by_timestamp_.emplace(ExpirationEvent{ now + 1000L, query_id, ExpirationKind::RESOURCE_LIMIT}); } 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 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 Ah, good catch. Yeah I think we should do something similar to the query timeouts where we re-enqueue it for a later time. expiration_event = queries_by_timestamp_.erase(expiration_event); queries_by_timestamp_.emplace(ExpirationEvent{ UnixMillis() + 1000L, query_id, ExpirationKind::RESOURCE_LIMIT}); I think otherwise we run into some potential fairness issues where we're always processing these resource limit checks before other time limits. -- 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: Fri, 01 Jun 2018 00:16:18 +0000 Gerrit-HasComments: Yes
