Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21653 )
Change subject: IMPALA-12737: Refactor the Workload Management Initialization Process. ...................................................................... Patch Set 37: (6 comments) There are some redundant includes. They don't really hurt anything, just pointing them out in case you were confused about how it works. http://gerrit.cloudera.org:8080/#/c/21653/36/be/src/service/workload-management-fields.cc File be/src/service/workload-management-fields.cc: http://gerrit.cloudera.org:8080/#/c/21653/36/be/src/service/workload-management-fields.cc@35 PS36, Line 35: #include "gen-cpp/Types_types.h" > That is where the "TPrimitiveType" struct is declared. It's already included in workload-management.h via query-record-state.h. http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management-init.cc File be/src/service/workload-management-init.cc: http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management-init.cc@29 PS37, Line 29: #include <boost/algorithm/string/case_conv.hpp> nit: unused http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management-init.cc@38 PS37, Line 38: #include "gen-cpp/SystemTables_types.h" nit: redundant, already included with workload-management.h http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management-init.cc@40 PS37, Line 40: #include "gen-cpp/Types_types.h" nit: redundant, already included with workload-management.h via query-state-record.h (wouldn't hurt to make it explicit in the header though) http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management-test.cc File be/src/service/workload-management-test.cc: http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management-test.cc@25 PS37, Line 25: #include "gen-cpp/SystemTables_types.h" nit: redundant, included in workload-management.h http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management.h File be/src/service/workload-management.h: http://gerrit.cloudera.org:8080/#/c/21653/37/be/src/service/workload-management.h@a28 PS37, Line 28: nit: this is included via query-state-record.h, but we do use a type from it (TPrimitiveType). Wouldn't hurt to be explicit, but it's also fine to leave it out. -- To view, visit http://gerrit.cloudera.org:8080/21653 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id645f94c8da73b91c13a23d7ac0ea026425f0f96 Gerrit-Change-Number: 21653 Gerrit-PatchSet: 37 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 28 Aug 2024 17:03:50 +0000 Gerrit-HasComments: Yes
