Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3535: Ignore invalid per-pool default query options ......................................................................
Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3056/1/be/src/service/query-options.cc File be/src/service/query-options.cc: Line 432: ignore_invalid_options > Not sure I follow. MergeStatus can deal with multiple errors and pass back Yeah you're right Line 451: if (ignore_invalid_options) { > The test to catch this bug is to add an incorrect query option key=option t In the case we expect this to error out, this would be false, so it will always hit l456 which is what we want. In the case we want to ignore errors (only the pool defaults, right now), this is true, so we never RETURN_IF_ERROR. So effectively it's the same behavior as just ignoring errors, but the bug is that we log this regardless. The test cases cover both of these cases wrt whether or not a failure was expected or not, we just don't have checks for the incorrect logging. However, I guess this is just too complicated... I'll just go with the MergeStatus approach. Anyway, I think I misunderstood what you were saying at first but I see what you're saying now, and I agree that is cleaner. -- To view, visit http://gerrit.cloudera.org:8080/3056 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If04733b775963091b0314c65286df126fd812358 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
