You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2024/08/05 19:50:15 UTC
[Impala-ASF-CR] IMPALA-12345: Add user quotas to Admission Control
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21616 )
Change subject: IMPALA-12345: Add user quotas to Admission Control
......................................................................
Patch Set 3:
(3 comments)
A few minor comments. I still have more to review, but won't get to it today.
http://gerrit.cloudera.org:8080/#/c/21616/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21616/3//COMMIT_MSG@24
PS3, Line 24: two reasons: (1) as queries can only be admitted from the front of the
nit: "as" is redundant at the beginning of the statement.
http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/common/global-flags.cc@193
PS3, Line 193:
nit: why add whitespace?
http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller-test.cc
File be/src/scheduling/admission-controller-test.cc:
http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller-test.cc@404
PS3, Line 404: DCHECK(false) << "unknown outcome";
This should be EXPECT_FALSE. clang-tidy flags this because it doesn't exit, and release build will likely fail to compile.
--
To view, visit http://gerrit.cloudera.org:8080/21616
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c33f3f2427db57fb9b6c593a4b22d5029549b41
Gerrit-Change-Number: 21616
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2024 19:50:15 +0000
Gerrit-HasComments: Yes