impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5282: Handle overflows in computeResourceProfile().
Date Mon, 05 Jun 2017 23:13:20 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5282: Handle overflows in computeResourceProfile().

Patch Set 1:

(1 comment)
File fe/src/main/java/org/apache/impala/service/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a83a76141853d3274f812e5a531f456e1b110b1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message