Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/24485 )
Change subject: IMPALA-15088: Add ReadOnlyIntGauge ...................................................................... Patch Set 1: (6 comments) Thanks for taking this on, just a few minor nits. Also, I recently merged https://github.com/apache/impala/commit/afc7224bd5a4257c9ce1a6f35141772fd3139838 , so there is an additional malloc-util file that will need to change. I will be offline until July 8th. http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/runtime/krpc-data-stream-mgr.h File be/src/runtime/krpc-data-stream-mgr.h: http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/runtime/krpc-data-stream-mgr.h@353 PS1, Line 353: ReadOnlyIntGauge* deserialization_queue_size_; For types that are subclasses of ReadOnlyIntGauge, let's leave the variable as the more specific type. To code using this, it is just as functional, but the more specific type allows for better compiler checks. http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/malloc-util-gperftools.h File be/src/util/malloc-util-gperftools.h: http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/malloc-util-gperftools.h@50 PS1, Line 50: class TcmallocMetric : public IntGauge { I would also make this inherit from ReadOnlyIntGauge. http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/malloc-util-libc.h File be/src/util/malloc-util-libc.h: http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/malloc-util-libc.h@33 PS1, Line 33: class LibcMallocMetric : public IntGauge { This can inherit from ReadOnlyIntGauge as it doesn't need to support the set functions. http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/malloc-util-sanitizers.h File be/src/util/malloc-util-sanitizers.h: http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/malloc-util-sanitizers.h@33 PS1, Line 33: class SanitizerMallocMetric : public IntGauge { This can inherit from ReadOnlyIntGauge as it doesn't need to support the set functions. http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/memory-metrics.h File be/src/util/memory-metrics.h: http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/memory-metrics.h@53 PS1, Line 53: static ReadOnlyIntGauge* TOTAL_USED; Since SumGauge now inherits from ReadOnlyIntGauge, I would leave this as the more specific type. http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/memory-metrics.cc File be/src/util/memory-metrics.cc: http://gerrit.cloudera.org:8080/#/c/24485/1/be/src/util/memory-metrics.cc@47 PS1, Line 47: ReadOnlyIntGauge* AggregateMemoryMetrics::TOTAL_USED = nullptr; See other comment about leaving this as a SumGauge type -- To view, visit http://gerrit.cloudera.org:8080/24485 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ida124aa3225ee07cf30e9f0a481477474a50c4dd Gerrit-Change-Number: 24485 Gerrit-PatchSet: 1 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Fri, 26 Jun 2026 19:13:34 +0000 Gerrit-HasComments: Yes
