Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16891 )
Change subject: IMPALA-9975 (part 2): Introduce new admission control daemon ...................................................................... Patch Set 1: (3 comments) Nice refactoring. I had a couple of high-level questions. http://gerrit.cloudera.org:8080/#/c/16891/1/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/16891/1/be/src/runtime/exec-env.cc@289 PS1, Line 289: if (FLAGS_is_coordinator && FLAGS_admission_control_service_addr.empty()) { IIRC the executors do actually need an AdmissionController to report the memory usage from the pool MemTrackers. Looking at the code there's a lot of indirection, but basically UpdateMemTrackerStats() gets the memory usage from the pool mem trackers and that gets added to the topic update. I think this is not as relevant as it once was, but it does play a role when we're running without memory limits or where there's significant untracked memory. So we want to preserve this behaviour at least in the standard config. We could consider disabling this when the admission control daemon is in use - it's less relevant when we don't need to deal with distributed admission control. http://gerrit.cloudera.org:8080/#/c/16891/1/be/src/scheduling/admissiond-main.cc File be/src/scheduling/admissiond-main.cc: http://gerrit.cloudera.org:8080/#/c/16891/1/be/src/scheduling/admissiond-main.cc@44 PS1, Line 44: ABORT_IF_ERROR(daemon_env.Init(/* init_jvm */ false)); Does the admission controller need to load the llama-site.xml and fair-scheduler.xml configs? Wondering if it might actually need a JVM for that purpose. http://gerrit.cloudera.org:8080/#/c/16891/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/16891/1/bin/start-impala-cluster.py@413 PS1, Line 413: 127.0.0.1 Maybe should be INTERNAL_LISTEN_HOST. Not sure it matters though. -- To view, visit http://gerrit.cloudera.org:8080/16891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id677814b31e9193035e8cf0d08aba0ce388a0ad9 Gerrit-Change-Number: 16891 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 22 Dec 2020 18:32:07 +0000 Gerrit-HasComments: Yes
