empiredan commented on code in PR #1304: URL: https://github.com/apache/incubator-pegasus/pull/1304#discussion_r1098143983
########## src/utils/metrics.cpp: ########## @@ -41,17 +48,15 @@ metric_entity::~metric_entity() close(close_option::kWait); } -void metric_entity::close(close_option option) +void metric_entity::close(close_option option) const { utils::auto_write_lock l(_lock); - // The reason why each metric is closed in the entity rather than in the destructor of each - // metric is that close() for the metric will return immediately without waiting for any close - // operation to be finished. - // - // Thus, to close all metrics owned by an entity, it's more efficient to firstly issue a close - // request for all metrics; then, just wait for all of the close operations to be finished. - // It's inefficient to wait for each metric to be closed one by one. + // To close all metrics owned by an entity, it's more efficient to firstly issue an asynchronous + // close request to each metric; then, just wait for all of the close operations to be finished. + // It's inefficient to wait for each metric to be closed one by one. Therefore, the metric is + // not + // closed in its destructor. for (auto &m : _metrics) { Review Comment: Compilation can be passed while this function is qualified with `const` since actually it does not modify the direct member (add, remove or update), such as `_metrics`; however, it will change the state of the member of `_metrics`. Therefore I think it's better to declare it without `const`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@pegasus.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@pegasus.apache.org For additional commands, e-mail: dev-h...@pegasus.apache.org