Tim Armstrong has posted comments on this change. Change subject: IMPALA-5282: Handle overflows in computeResourceProfile(). ......................................................................
Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7084/1/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: Line 1019: try { > I agree that this try/catch is a crutch for missing tests. We did rely on the estimates for admission control before when query mem limits were not set, there was just logic that worked around the estimates being not set. This overflow bug was present before the resource profile change but was partially masked. Instead of an error there would have been a warning and admission control would use --rm_default_memory. My concern is that hiding a bug in a warning message will just prevent us from finding the bugs until something else breaks as an indirect consequence. Part of my goal with that change was to get it in before the buffer pool stuff and make sure that the invariants actually held before we built more things on top of it. E.g. without the invariant check there's no way the random query generator can find bugs like this. My preference is to fix this specific bug and leave it at that, unless there's a specific gap in test coverage that we need more time to address. If we don't have a specific goal for the test coverage we need we'll just be giving the code opportunity to bit-rot before it gets turned on again for the buffer pool work. If we really needed to do that we could add code that marked the estimates as invalid and fell back to --rm_default_memory. -- To view, visit http://gerrit.cloudera.org:8080/7084 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8a83a76141853d3274f812e5a531f456e1b110b1 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
