gortiz commented on code in PR #13035: URL: https://github.com/apache/pinot/pull/13035#discussion_r1628906266
########## pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/MultiStageQueryStats.java: ########## @@ -284,14 +284,18 @@ public void mergeUpstream(List<ByteBuffer> otherStats) { myStats.merge(dis); } } catch (IOException ex) { - LOGGER.warn("Error deserializing stats on stage {}. Considering the new stats empty", i, ex); + LOGGER.warn("Error deserializing stats on stage " + i + ". Considering the new stats empty", ex); } catch (IllegalArgumentException | IllegalStateException ex) { - LOGGER.warn("Error merging stats on stage {}. Ignoring the new stats", i, ex); + LOGGER.warn("Error merging stats on stage " + i + ". Ignoring the new stats", ex); } } } } + public List<StageStats.Closed> getClosedStats() { + return Collections.unmodifiableList(_closedStats); Review Comment: A unmodifiable list is very light (just an indirection) and TBH I prefer to be a bit paranoic here so we don't end up modifying it in the future, which would be difficult to catch. This method is only called once per query on the broker. I don't think creating a single small object (that can be probably scalar replaced) would be problematic. -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org