[Impala-ASF-CR] IMPALA-7764: Improve SentryProxy test coverage
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11880 ) Change subject: IMPALA-7764: Improve SentryProxy test coverage .. Patch Set 6: Code-Review+2 (2 comments) just minor suggestions from my end to clarify the tests. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java File fe/src/test/java/org/apache/impala/util/SentryProxyTest.java: http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@242 PS3, Line 242: server=server1->d > It's just a good additional test case usually when dealing with case. I can its fine to keep it. I just saw it used in one place, to retrieve the role from the policy, as opposed to the more comprehensive treatment that upper-case gets. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@291 PS3, Line 291: > If we just used one privilege, we wouldn't know if the user was assigned to ok, add a brief comment to clarify the purpose of these differences. -- To view, visit http://gerrit.cloudera.org:8080/11880 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1190638283f051cc37fda7ea5dd0c5e8a70d40db Gerrit-Change-Number: 11880 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 14 Nov 2018 00:28:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7764: Improve SentryProxy test coverage
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11880 ) Change subject: IMPALA-7764: Improve SentryProxy test coverage .. Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java File fe/src/test/java/org/apache/impala/util/SentryProxyTest.java: http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@94 PS3, Line 94: nit: use consistent spacing for these. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@100 PS3, Line 100: false this (resetVersions) seems to have caused issues yet all of the tests here consider only the false case. worth adding tests when set to true? from looking at that code, checking that updates get a version change (on reset) would test priv name construction and its lookup at the least. http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@242 PS3, Line 242: mixedCaseRoleName what's the point of this one? http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@261 PS3, Line 261: lowerCaseRoleName.toUpperCase() just use the upperCaseRoleName? http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@266 PS3, Line 266: SentryProxy.refreshSentryAuthorization(catalog, sentryService_, USER, false); what is the purpose of this refresh? the operation before is expected to fail (L261). http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@291 PS3, Line 291: "functional_kudu" these are all different users, so why the different privs? http://gerrit.cloudera.org:8080/#/c/11880/3/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@295 PS3, Line 295: Impala stores the role name in case insensitive way. expecting this to be about users. -- To view, visit http://gerrit.cloudera.org:8080/11880 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1190638283f051cc37fda7ea5dd0c5e8a70d40db Gerrit-Change-Number: 11880 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 09 Nov 2018 00:28:27 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7835: Role and user catalog objects with the same name can cause INVALIDATE METADATA to hang
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11910 ) Change subject: IMPALA-7835: Role and user catalog objects with the same name can cause INVALIDATE METADATA to hang .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I516cf72e69e142a1349950cfca91f035c1ed445f Gerrit-Change-Number: 11910 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 08 Nov 2018 22:45:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7835: Role and user catalog objects with the same name can cause INVALIDATE METADATA to hang
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11910 )
Change subject: IMPALA-7835: Role and user catalog objects with the same name
can cause INVALIDATE METADATA to hang
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11910/5/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/11910/5/be/src/catalog/catalog-util.cc@206
PS5, Line 206: // The format is .
> Yeah that's how the catalog determines the object_type. So the first part o
ok, add a pointer to the java code to make it easier to track down how this is
created.
http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Catalog.java@568
PS5, Line 568: return "PRINCIPAL:" + principalName + "." +
> Yup, this is definitely a repetition. With the existing design, it can take
that's fine. pls add a todo/jira.
http://gerrit.cloudera.org:8080/#/c/11910/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11910/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@564
PS6, Line 564: String principalName =
simpler?
String principalName = catalogObject.getPrincipal().getPrincipal_name();
if (catalogObject.getPrincipal().getPrincipal_type() == TPrincipalType.ROLE) {
principalName = principalName.toLowerCase();
}
http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Principal.java
File fe/src/main/java/org/apache/impala/catalog/Principal.java:
http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Principal.java@163
PS5, Line 163: getName().toLowerCase()
> It's because user name is supposed to be case sensitive. In other words, `f
makes sense.. that sounds like further fallout from that case-sensitivity
issue. I'm ok with making these two fixes here, but just wanted to clarify that
this issue (case sensitivity) is not the cause of this particular hang.
--
To view, visit http://gerrit.cloudera.org:8080/11910
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I516cf72e69e142a1349950cfca91f035c1ed445f
Gerrit-Change-Number: 11910
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 08 Nov 2018 21:32:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7835: Role and user catalog objects with the same name can cause INVALIDATE METADATA to hang
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11910 ) Change subject: IMPALA-7835: Role and user catalog objects with the same name can cause INVALIDATE METADATA to hang .. Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/11910/5/be/src/catalog/catalog-util.cc File be/src/catalog/catalog-util.cc: http://gerrit.cloudera.org:8080/#/c/11910/5/be/src/catalog/catalog-util.cc@206 PS5, Line 206: // The format is . is this what Catalog.java is producing? If so, its ok that principal name be prefixed with a "PRINCIPAL:" string? http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Catalog.java@568 PS5, Line 568: return "PRINCIPAL:" + principalName + "." + why is getUniqueName not used here? if it can't be, this looks ripe for factoring. http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Principal.java File fe/src/main/java/org/apache/impala/catalog/Principal.java: http://gerrit.cloudera.org:8080/#/c/11910/5/fe/src/main/java/org/apache/impala/catalog/Principal.java@163 PS5, Line 163: getName().toLowerCase() I see this is done just for the role case now, why? I understood the issue here to be that a lack of a principal type caused unintentional duplication. http://gerrit.cloudera.org:8080/#/c/11910/5/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11910/5/tests/authorization/test_grant_revoke.py@351 PS5, Line 351: # Verify that running invalidate metadata won't hang due having the same name nit: missing "to" -- To view, visit http://gerrit.cloudera.org:8080/11910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I516cf72e69e142a1349950cfca91f035c1ed445f Gerrit-Change-Number: 11910 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 08 Nov 2018 20:01:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7764: Improve SentryProxy test coverage
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11880 )
Change subject: IMPALA-7764: Improve SentryProxy test coverage
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11880/1/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
File fe/src/test/java/org/apache/impala/util/SentryProxyTest.java:
http://gerrit.cloudera.org:8080/#/c/11880/1/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java@89
PS1, Line 89: public void testRefreshRolePrivileges() throws ImpalaException {
this is a great improvement to make the proxy easier to test. currently, there
seems to be a fair bit of boiler-plate to setup state on the catalog, on
sentry, then check. perhaps this can be abstracted a bit, something like this:
setCatalogRoles( [ {ADD, updateRole, ["functional"]},
{ADD, removeRole, ["functional"]} ])
setSentryStateRoles( [ {ADD, addRole, ["functional"]},
{ADD, updateRole, ["functional",
"functional_kudu"]},
{DROP, removeRole}])
refresh()
checkCatalogRoles([{updateRole, ["functional",
"functional_kudu"]},
{addRole, ["functional"]}])
representation can differ a bit, but the idea is to make it easier to try
different combinations. examples:
- explicit test for empty case
- explicit tests for add, delete, update
- non-additive updates
- non-empty state to empty state to non-empty state
- sentry drop of a role not in the catalog
- any limits for number of roles or privs?
this operation is basically trying to merge two collections, so coverage of
these various states would be useful.
--
To view, visit http://gerrit.cloudera.org:8080/11880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1190638283f051cc37fda7ea5dd0c5e8a70d40db
Gerrit-Change-Number: 11880
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:51:02 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11887 ) Change subject: IMPALA-7818: Standardize use of Expr predicates .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@132 PS4, Line 132: IS_NON_NULL_LITERAL > I agree, the original is confusing with "isNullLiteral()" meaning "null lit makes sense, thanks. -- To view, visit http://gerrit.cloudera.org:8080/11887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a Gerrit-Change-Number: 11887 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Nov 2018 21:35:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7824: INVALIDATE METADATA should not hang when Sentry is unavailable
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11897 ) Change subject: IMPALA-7824: INVALIDATE METADATA should not hang when Sentry is unavailable .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icff987a6184f62a338faadfdc1a0d349d912fc37 Gerrit-Change-Number: 11897 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Nov 2018 20:59:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11887 ) Change subject: IMPALA-7818: Standardize use of Expr predicates .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11887/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11887/4//COMMIT_MSG@10 PS4, Line 10: isMuble() sp? -- To view, visit http://gerrit.cloudera.org:8080/11887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a Gerrit-Change-Number: 11887 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Nov 2018 19:34:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11887 ) Change subject: IMPALA-7818: Standardize use of Expr predicates .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@1169 PS4, Line 1169: Expr. drop here, and other places where its not needed. http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@432 PS4, Line 432: Expr. can drop http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@132 PS4, Line 132: IS_NON_NULL_LITERAL double-checking if this translation preserves the previous expression? isNullLiteral is replaced by the value variant but IS_NON_NULL_LITERAL uses the instanceof check. I can be convinced that it falls out to be the same, but it is not obvious. http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java File fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java@52 PS4, Line 52: IS_LITERAL_VALUE IS_LITERAL? -- To view, visit http://gerrit.cloudera.org:8080/11887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a Gerrit-Change-Number: 11887 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Nov 2018 19:32:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 ) Change subject: IMPALA-7808: Refactor Analyzer for easier debugging .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52 Gerrit-Change-Number: 11883 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:03:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 ) Change subject: IMPALA-7808: Refactor Analyzer for easier debugging .. Patch Set 2: Code-Review+2 go ahead and apply the correct indentation. -- To view, visit http://gerrit.cloudera.org:8080/11883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52 Gerrit-Change-Number: 11883 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 21:20:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..
Patch Set 1:
(8 comments)
thanks for the change and making it easier to review. mostly minor comments
from my end.
http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG@37
PS1, Line 37: Testing
pls move this testing section above the Change-Id
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@176
PS1, Line 176: private final Analyzer analyzer;
these members should be suffixed with "_"
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@186
PS1, Line 186: Note tem
note, you can always include such review-level comments as comments in gerrit.
no need for source changes to communicate these things.
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389
PS1, Line 389: protected
did some other class need this protected?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@617
PS1, Line 617: public
why public?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@659
PS1, Line 659: public
why public?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@757
PS1, Line 757: public void verifyAggregation() throws AnalysisException {
why public?
http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@894
PS1, Line 894: }
just to remind myelf... end of inner class.
--
To view, visit http://gerrit.cloudera.org:8080/11883
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 06 Nov 2018 17:58:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 ) Change subject: IMPALA-5031: memcpy cannot take null arguments .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51 Gerrit-Change-Number: 11812 Gerrit-PatchSet: 1 Gerrit-Owner: Jim Apple Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 03 Nov 2018 17:45:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7794: Rewrite flaky ownership authorization tests
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11837 ) Change subject: IMPALA-7794: Rewrite flaky ownership authorization tests .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11837 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic98f8dbec41360261fd0339d835f3ce6b504ee29 Gerrit-Change-Number: 11837 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 02 Nov 2018 05:32:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 6: Code-Review+2 lgtm from my end. will let others on the thread have another look. for maintenance, I'd go with a follow-up to remove ParseNode's toSql(). then for the cases that were default, I'd look if they're really "toSql" or some other type of to-string method. for the non-parseNode types, "toSql" is fine... the differing type guards against the maintenance issue. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 22:27:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 5: (4 comments) While here, do you have thoughts on how to maintain this going forward? Main concern is that a "toSql()" (default options) will be used and we'll forget to look for casts, if expected. I see that "toSql()" is used in many places other than this use-case so I'd lean towards follow-up changes to require the option and see if we can replace the other "toSql()" calls with something more explicit for their use. I'm fine with a todo/jira. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 > Just some padding for if lines expand. Maybe this is too ugly. ok, just mention it in a brief comment. I prefer such inline constants to have some explanation. It seems to be an optimization so I'm also ok with removing it to simplify. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: col > I don't really understand what is wrong with param? nothing's wrong with it. my comments are motivated to keep things consistent. http://gerrit.cloudera.org:8080/#/c/11719/5/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/5/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@89 PS5, Line 89: * nit: looks like there's still an extra '*' here. http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test File testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test: http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@9 PS4, Line 9: -- +straight_join : * > Yes. Though I welcome your opinion. that's fine by me -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 16:21:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11760 ) Change subject: IMPALA-7655: Rewrite if, isnull to use CASE .. Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG@42 PS11, Line 42: While the desire was to replace the BE implementation of if and isnull, This section summarizes limitations. They are: (1) no guarantee that the fns are removed (list dependent bugs, disabled rewrites), (2) missed simplifications (list dependent bugs). I think this can be made more concise. http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG@45 PS11, Line 45: ORDER BY clause, and 2) if the user disables rewrites. prior, you mentioned a potential performance regression-- I think I saw an example of this in the current tests, pls confirm. http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG@50 PS11, Line 50: Coalesce() is implemented, but disabled. The old rewrites are retained : until b as mentioned in the source files, I'd remove this. http://gerrit.cloudera.org:8080/#/c/11760/11/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test File testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test: http://gerrit.cloudera.org:8080/#/c/11760/11/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test@2238 PS11, Line 2238: int_col did you want to mention the repeated sub-expression evaluation for examples like this (int_col could be an expression)? -- To view, visit http://gerrit.cloudera.org:8080/11760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 Gerrit-Change-Number: 11760 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 31 Oct 2018 23:53:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11760 ) Change subject: IMPALA-7655: Rewrite if, isnull to use CASE .. Patch Set 11: (13 comments) http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java File fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java: http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@39 PS11, Line 39: coalesce(v1, v2, ...) pls remove. here's a good place to put one todo for the coalesce work. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@43 PS11, Line 43: nullif where's this handled? http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@65 PS11, Line 65: coalesce stale. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@99 PS11, Line 99: // IMPALA-7793: CASE statement does not handle NULL from UDF overflow : // Thus, CASE cannot be made to act as coalesce() when handling Decimal : // overflow, so can't do the rewrite. : //case "coalesce": : // return rewriteCoalesceFn(expr); lets get rid of this. example changes for coalesce can be revived from this review. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@164 PS11, Line 164: rewriteCoalesceFn pls remove. http://gerrit.cloudera.org:8080/#/c/11760/10/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: http://gerrit.cloudera.org:8080/#/c/11760/10/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@217 PS10, Line 217: // Note: retaining the current simplification because of bug: : // IMPALA-7793: CASE statement does not handle NULL from UDF overflow : // Which makes it impossible to rewrite coalesce() to CASE in : // RewriteConditionalFnsRule without loss of functionality. : // pls remove and optionally put this as a todo with the new rewrites. when coalesce is properly removed, removing this will follow. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@a587 PS11, Line 587: why are these removed? http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@296 PS11, Line 296: // Note: retaining the current simplification because of bug: : // IMPALA-7793: CASE statement does not handle NULL from UDF overflow : // Which makes it impossible to rewrite coalesce() to CASE in : // RewriteConditionalFnsRule without loss of functionality. : // This implementation is temporary until that bug is fixed. : // Once it is, remove these tests and enable those in : // RewriteConditionalFnsRuleTest and FullRewriteTest. pls remove. this is more of a note for the reviewer and not necessary to stick with the code as currently written. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java File fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java: http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@37 PS11, Line 37: r coalesce since its rewrite handles all the : * required simplification. stale http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@119 PS11, Line 119: //v thank you for including tests that should work for bugs that will be fixed later. nit: pls turn these into block comments so that they are not at the start of the line. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@163 PS11, Line 163: i nit: capitalize http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@223 PS11, Line 223: TestNullif() what's going on with the duplication with the test on L240? http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@257 PS11, Line 257: Where were you planning to have a standard battery of expressions that you then apply to different parts of the statement? it does not
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: (15 comments) overall good cleanup. mostly minor comments from my end. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: sql nit: pls use consistent case (SQL, sql, Sql) http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: @param pls omit these javadoc annotations. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@39 PS4, Line 39:* This should return the same result as calling toSql(ToSqlOptions.DEFAULT). would be good to do this via a default interface method. pls add a todo for when we fully move to Java 8. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@25 PS4, Line 25: default, normal "normal" makes sense if you know what is currently done. perhaps "original query without rewrites"? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: // This does have the consequence that the sql with implict casts may not pssibly fail nit: spelling http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: may not pssibly fail : // to parse if resubmitted as I found this difficult to understand. Did you mean that Sql produced in this mode may not be valid Sql? I'd prefer the comment be merged with the comment block on L35. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java File fe/src/main/java/org/apache/impala/analysis/TypeDef.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java@165 PS4, Line 165: return parsedType_.toSql(); pass down here? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 what's this? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@115 PS4, Line 115: exiting sp. existing? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: @pa remove http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@451 PS4, Line 451: // AnalyzesOk(stmt.toSql(true), ctx); rm? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@750 PS4, Line 750: @para remove the comment annotations. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@801 PS4, Line 801: This would also be included if both more direct: Equivalent to enabling INCLUDE_EXPLAIN_HEADER and EXTENDED_EXPLAIN. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@128 PS4, Line 128: @pa rm http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test File testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test: http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@9 PS4, Line 9: -- +straight_join : * were you expecting the hint to print this way? -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType:
[Impala-ASF-CR] IMPALA-7614: [DOCS] Part 2: Document the New Invalidate option
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11839 ) Change subject: IMPALA-7614: [DOCS] Part 2: Document the New Invalidate option .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11839 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a825bff4a09a66b28138a8ece4ee6a60868b189 Gerrit-Change-Number: 11839 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adrian Ng (389) Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 31 Oct 2018 19:05:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7614: [DOCS] Document the New Invalidate Options
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11809 ) Change subject: IMPALA-7614: [DOCS] Document the New Invalidate Options .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11809/3/docs/topics/impala_config_options.xml File docs/topics/impala_config_options.xml: http://gerrit.cloudera.org:8080/#/c/11809/3/docs/topics/impala_config_options.xml@355 PS3, Line 355: small bounded http://gerrit.cloudera.org:8080/#/c/11809/3/docs/topics/impala_config_options.xml@365 PS3, Line 365: Java garbage collection-based simplify to: Memory-based http://gerrit.cloudera.org:8080/#/c/11809/3/docs/topics/impala_config_options.xml@373 PS3, Line 373: but the feature could potentially : cause performance risks do we have standardized phrasing around memory knobs? something like, "may require tuning". -- To view, visit http://gerrit.cloudera.org:8080/11809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40c552eeaee81ee6528d9f725bd416b51d8ab837 Gerrit-Change-Number: 11809 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adrian Ng (389) Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 31 Oct 2018 05:57:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11760 ) Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE .. Patch Set 9: (10 comments) still reviewing the tests. http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG@43 PS8, Line 43: (IMPALA-7737) are there examples of these fns in our benchmarks to quantify the regression? if so, would be useful to see the effect. http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG@45 PS8, Line 45: Still, the fix provides most of what the JIRA ticket requested I'd skip these next three paragraphs. http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG@57 PS8, Line 57: pls make a section for this called "Testing" so its easier to jump to. also, pls condense these so that they're easier to skim. for example: - split up tests for conditional functions to make them easier to test - added unit tests for end-to-end rewrite rule interactions - updated existing planner tests due to rewrites http://gerrit.cloudera.org:8080/#/c/11760/8/be/src/exprs/conditional-functions.h File be/src/exprs/conditional-functions.h: http://gerrit.cloudera.org:8080/#/c/11760/8/be/src/exprs/conditional-functions.h@76 PS8, Line 76: since : /// various bugs mean that this implementation is still sometimes used. But : /// the goal is to remove these classes at some point. simpler: "until their use is eliminated by the frontend". http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java File fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java: http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@37 PS8, Line 37: vanish is this accurate given the comments in the commit message about order by? http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@48 PS8, Line 48: planner runs the rule to simplify CASE : * after this rule. Where that other rule can perform simplifications, : * those simplifications are omitted here simplify and use the specific rule name for concreteness. http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@106 PS8, Line 106: return rewriteIfNullFn(expr) clarify whether you think this happens after the rewrite or before. If its after, then I expect the example on L109,110 to be in terms of CASE. I'm also fine with omitting the example since its assumed that these rules compose. http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@130 PS8, Line 130:expr.get isn't this all that's done here (the most general case) and we'll depend on other rewrites for further simplifications? http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@172 PS8, Line 172: The simplest rewrite here would be to not look at the child exprs for the various scenarios listed above and instead simply translate naively to a case statement. From there, we'd get constant folding and case simplification which will find the first when clause that evals to true. preceding when clauses that remain unknown will be retained, but this transform will need to retain them as well. Aggregate handling will result in a brute-force roll-back of the rewrite in case simplification, which will result in falling back to the case rewrite here. Might want to handle that situation by retaining the coalesce for now. So besides that issue, what else do we miss by doing the simple thing and rely on case simplification? http://gerrit.cloudera.org:8080/#/c/11760/8/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test File testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test: http://gerrit.cloudera.org:8080/#/c/11760/8/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1137 PS8, Line 1137: | other predicates: functional.alltypestiny.tinyint_col + functional.alltypestiny.smallint_col + functional.alltypestiny.int_col > 10, CASE WHEN functional.alltypestiny.tinyint_col + functional.alltypestiny.bigint_col IS NULL THEN 1 ELSE functional.alltypestiny.tinyint_col + functional.alltypestiny.bigint_col END = 1 so this is the example of the performance regression (same work on multiple when clauses)? -- To view, visit http://gerrit.cloudera.org:8080/11760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 ) Change subject: IMPALA-5031: memcpy cannot take null arguments .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11812/1/be/src/runtime/string-buffer.h File be/src/runtime/string-buffer.h: http://gerrit.cloudera.org:8080/#/c/11812/1/be/src/runtime/string-buffer.h@93 PS1, Line 93: memcpy I see new_buffer is guarded here. Is this one not replaced bc buffer_ is assumed to not be null here? Haven't looked too closely, but it looks like buffer_ could be null. -- To view, visit http://gerrit.cloudera.org:8080/11812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51 Gerrit-Change-Number: 11812 Gerrit-PatchSet: 1 Gerrit-Owner: Jim Apple Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Mon, 29 Oct 2018 18:57:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7742: Store the Sentry user names in a case sensitive way
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11762 ) Change subject: IMPALA-7742: Store the Sentry user names in a case sensitive way .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04bec045e3f70fc4f41b16b9b5c55eeb60bd63b8 Gerrit-Change-Number: 11762 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 26 Oct 2018 23:32:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7742: Stores the Sentry user names in a case sensitive way
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11762 ) Change subject: IMPALA-7742: Stores the Sentry user names in a case sensitive way .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11762/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/11762/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@80 PS2, Line 80: false something seems out of alignment here and http://github.mtv.cloudera.com/CDH/Impala/blob/67bde8e6b2ed758de02ef04ec27b15a4a3bc8a5c/fe/src/main/java/org/apache/impala/catalog/Principal.java#L42 fwict, we have CatalogObjectCaches for users, roles (in authorization policy), and abstractly their privs in the super-class, Principals. Here, users are keyed case sensitive whereas in Principals, privs are keyed case insensitive. why's that? http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py@418 PS1, Line 418: FOOBAR_impalad_client = self.create_impala_client() > Yeah it's a bit weird but that's because a session is a connection to a par that's for running against test files. what breaks if the same client is used? if that "user" param modifies some state (e.g., on L425), that's not obvious. -- To view, visit http://gerrit.cloudera.org:8080/11762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04bec045e3f70fc4f41b16b9b5c55eeb60bd63b8 Gerrit-Change-Number: 11762 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 26 Oct 2018 20:57:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7742: Stores the Sentry user names in a case sensitive way
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11762 ) Change subject: IMPALA-7742: Stores the Sentry user names in a case sensitive way .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/11762/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11762/1//COMMIT_MSG@11 PS1, Line 11: stores nit: store http://gerrit.cloudera.org:8080/#/c/11762/1/fe/src/main/java/org/apache/impala/catalog/User.java File fe/src/main/java/org/apache/impala/catalog/User.java: http://gerrit.cloudera.org:8080/#/c/11762/1/fe/src/main/java/org/apache/impala/catalog/User.java@41 PS1, Line 41: true isn't this the one that needs to be switched to false? does it make sense to have a public static method for this... the protected method here can just use it, and it would be it more obvious when initializing the cache in AuthorizationPolicy. There you would use something like... new Cache...(User.isCaseInsensitiveKeys()) http://gerrit.cloudera.org:8080/#/c/11762/1/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java: http://gerrit.cloudera.org:8080/#/c/11762/1/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@59 PS1, Line 59: FOOBAR what is being tested with case sensitive group names? http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py@414 PS1, Line 414: cases nit: case http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py@418 PS1, Line 418: FOOBAR_impalad_client = self.create_impala_client() why are two clients needed? -- To view, visit http://gerrit.cloudera.org:8080/11762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04bec045e3f70fc4f41b16b9b5c55eeb60bd63b8 Gerrit-Change-Number: 11762 Gerrit-PatchSet: 1 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 26 Oct 2018 18:26:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7760: Privilege version inconsistency causes a hang when running invalidate metadata
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11794 ) Change subject: IMPALA-7760: Privilege version inconsistency causes a hang when running invalidate metadata .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib1e0db2b1f727476f489c732c4f4e5bc1582429f Gerrit-Change-Number: 11794 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 26 Oct 2018 16:41:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11786 ) Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11786 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219 Gerrit-Change-Number: 11786 Gerrit-PatchSet: 4 Gerrit-Owner: Adam Holley Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 26 Oct 2018 16:41:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11786 ) Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11786/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11786/3//COMMIT_MSG@15 PS3, Line 15: updated. something here is unclear about whether the feature is working as intended but the test was incorrect or if something needs to improve w/ the feature (perhaps link to the jira that discusses solving some of these issues w/ a lock?). -- To view, visit http://gerrit.cloudera.org:8080/11786 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219 Gerrit-Change-Number: 11786 Gerrit-PatchSet: 3 Gerrit-Owner: Adam Holley Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 25 Oct 2018 21:16:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6323 Allow constant analytic window expressions.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11556 )
Change subject: IMPALA-6323 Allow constant analytic window expressions.
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11556/5/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java:
http://gerrit.cloudera.org:8080/#/c/11556/5/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@91
PS5, Line 91: {
remove. as in L89
http://gerrit.cloudera.org:8080/#/c/11556/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java:
http://gerrit.cloudera.org:8080/#/c/11556/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@91
PS6, Line 91: { return true; }
nit: if we're making any changes, pls remove these braces as mentioned earlier.
--
To view, visit http://gerrit.cloudera.org:8080/11556
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf129026c45120e9470df601268863634037908c
Gerrit-Change-Number: 11556
Gerrit-PatchSet: 6
Gerrit-Owner: Michal Ostrowski
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michal Ostrowski
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 25 Oct 2018 19:11:03 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5004: Switch to sorting node for large TopN queries
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11698 )
Change subject: IMPALA-5004: Switch to sorting node for large TopN queries
..
Patch Set 5: Code-Review+1
(2 comments)
one question about tests, but otherwise, lgtm. will let others take another
pass.
http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:
http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@334
PS4, Line 334: then it is likely we are sor
> Yeah, but that doesn't seem ideal. So I the most recent patch allows settin
makes sense, that seems easier.
http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:
http://gerrit.cloudera.org:8080/#/c/11698/5/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@214
PS5, Line 214: public void testTopNBytesLimitDisabled() {
perhaps merge this with the test on L195? settings are the same so unclear why
these are separated.
--
To view, visit http://gerrit.cloudera.org:8080/11698
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34c9db33c9302b55e9978f53f9c7061f2806c8a9
Gerrit-Change-Number: 11698
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 25 Oct 2018 04:20:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11760 ) Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE .. Patch Set 4: (3 comments) main question from my end is to consider what happens when rewrites are disabled. http://gerrit.cloudera.org:8080/#/c/11760/4/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/11760/4/common/function-registry/impala_functions.py@531 PS4, Line 531: # Rewritten away in the FE can you explain why this is needed in this change? do we assert anywhere that these fns are never seen by the backend? http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@a92 PS4, Line 92: rewrites can be disabled via a query option so cannot be assumed to run. it cannot be assumed to transform an invalid/unknown stmt to a valid one. its purpose is only to transform valid statements to other equivalent valid statements. what happens when you do: set enable_expr_rewrites = false ? http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@112 PS4, Line 112: to be consistent with the style in the frontend Java code, pls remove these javadoc markups. I think the idea is that the code is read through a variety of editors and since the javadocs are not used, simplify writing/reading the comments by omitting the markup. -- To view, visit http://gerrit.cloudera.org:8080/11760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 Gerrit-Change-Number: 11760 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 24 Oct 2018 21:13:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5004: Switch to sorting node for large TopN queries
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11698 ) Change subject: IMPALA-5004: Switch to sorting node for large TopN queries .. Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@236 PS4, Line 236: is the http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@238 PS4, Line 238:* average tuple serialized size. offset is not mentioned so either include it or remove it since the arg descriptions and the one-liner definition make things pretty clear already. http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@240 PS4, Line 240: @param Even though this style of commenting is widely used, Impala tries not to use it (yes, there are exceptions in the code base, but its on the rare side). If a file already uses them, then pls stick with the style of that file, but in this case, lets stick with the default style so remove the '@param'. http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@325 PS4, Line 325: estimateTopNMaterializedSize as pointed out in the header for SortInfo, the abstractions are not quite right. however, this call-site looks fairly complicated. compueMemLayout does not redo if its already been called, so perhaps simplify this to let SortInfo deal with its descriptors and calling computeMemLayout? You can have a static method do what estimateTopNMaterializedSize does now so that it can be used in SortInfo as well as from the SortNode. http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@326 PS4, Line 326: getAvgSerializedSize() this looks correct and consistent with the avgRowSize_ computation for PlanNodes. http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@334 PS4, Line 334: estimatedTopNMaterializedSize if we get this wrong, perhaps due to a mistaken many-to-many join cardinality estimate in some descendant child, what is the easiest way to turn this decision off? set topn_bytes_limit to max long? http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/11698/4/fe/src/main/java/org/apache/impala/planner/SortNode.java@260 PS4, Line 260: nodeResourceProfile_ = ResourceProfile.noReservation( where'd the return go? pls look into why tests did not get bothered by this... the profile could have been overwritten down below, in which case we'd get possibly different estimates. http://gerrit.cloudera.org:8080/#/c/11698/4/testdata/workloads/functional-planner/queries/PlannerTest/topn-default-limit-bytes.test File testdata/workloads/functional-planner/queries/PlannerTest/topn-default-limit-bytes.test: http://gerrit.cloudera.org:8080/#/c/11698/4/testdata/workloads/functional-planner/queries/PlannerTest/topn-default-limit-bytes.test@1 PS4, Line 1: triggers sort seems to apply to the second test, not the first, so got confused by this. -- To view, visit http://gerrit.cloudera.org:8080/11698 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34c9db33c9302b55e9978f53f9c7061f2806c8a9 Gerrit-Change-Number: 11698 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 24 Oct 2018 17:19:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7697: Fix flakiness in test resource limits
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11741 ) Change subject: IMPALA-7697: Fix flakiness in test_resource_limits .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11741/1/testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test File testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test: http://gerrit.cloudera.org:8080/#/c/11741/1/testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test@46 PS1, Line 46: select sleep(1) I like that the simplification addresses more directly what's being tested here. Can we do the same for the other time limit tests here? -- To view, visit http://gerrit.cloudera.org:8080/11741 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ba7080f62f0af3e16ef6c304463ebf78dec1b0c Gerrit-Change-Number: 11741 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 22:28:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7699: Fix spilling test run with hdfs erasure coding turned on
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11740 ) Change subject: IMPALA-7699: Fix spilling test run with hdfs erasure coding turned on .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I207569822ba7388e78936d25e2311fa09c7a1b9a Gerrit-Change-Number: 11740 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 22:24:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11721 ) Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7 Gerrit-Change-Number: 11721 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 20:21:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11721 ) Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/11721/8/be/src/catalog/catalog-util.cc File be/src/catalog/catalog-util.cc: http://gerrit.cloudera.org:8080/#/c/11721/8/be/src/catalog/catalog-util.cc@227 PS8, Line 227: stoi(principal_id)) lets use StringParser::StringToInt here (util/string-parser.h) -- To view, visit http://gerrit.cloudera.org:8080/11721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7 Gerrit-Change-Number: 11721 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 20:02:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11721 )
Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a
privilege
..
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util-test.cc
File be/src/catalog/catalog-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util-test.cc@122
PS6, Line 122: TPrivilege privilege;
add more edge cases for these negative tests. for example, empty string,
unexpected lengths, mixed case. as it stands, I don't trust that string
(where's it come from?)
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210
PS6, Line 210: privilege
principal
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210
PS6, Line 210: privilege
principal
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@226
PS6, Line 226: atoi(principal_id.c_str())
what does this do for a malformed id?
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@227
PS6, Line 227: if (principal_type == "ROLE") {
guaranteed to be uppercase?
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@248
PS6, Line 248: TPrivilege* privilege
dcheck not null for this. its a publicly exposed method (for test only?)
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@250
PS6, Line 250: boost::algorithm::split_regex(split, object_name,
boost::regex("->"));
add a comment about the expected format and an example.
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@253
PS6, Line 253: key_value, s, [](char c){ return c == '='; });
: if (key_value[0]
check the len... valid?
http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@254
PS6, Line 254: "server"
all guaranteed to be lowercase?
--
To view, visit http://gerrit.cloudera.org:8080/11721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 19 Oct 2018 18:38:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11734 ) Change subject: IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name .. Patch Set 8: Code-Review+2 thx for the fix! -- To view, visit http://gerrit.cloudera.org:8080/11734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 17:59:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11734 ) Change subject: IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name .. Patch Set 7: looks fine.. just one more clarification for the test. -- To view, visit http://gerrit.cloudera.org:8080/11734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 17:27:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11734 ) Change subject: IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11734/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11734/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@199 PS7, Line 199: // sentryRole.getRoleName() always returns the role name in lower case. : // However role.getName() preserves the case sensitivity of the role name. : // It is important to get the set of privileges from allRolesPrivileges using : // sentryRole.getRoleName() instead of role.getName(). If Sentry changes : // the role names to be upper case or case sensitive, we don't need to : // change this particular code since the case for allRolesPrivileges' keys : // will always match sentryRole.getRoleName(). terser at the call-sites: allRolesPrivileges keys and sentryRole are used here since they both come from Sentry so agree in case. http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@388 PS4, Line 388: tadata won't hang due > Done what was done? -- To view, visit http://gerrit.cloudera.org:8080/11734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 17:27:14 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11734 ) Change subject: IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@172 PS4, Line 172: String > This is lower case as of now. However, it's probably a good idea to not ass got it. keeping track of what's from sentry and what isn't and how that may or may not change over time is error prone. how about we just canonicalize up-front here to lower-case strings? perhaps a cleaner option is just have stuff that comes back from Sentry conform to data structures used in Impala. So in this case, that Map could be a CatalogObjectCache. If you canonicalize the map strings, then you'd need to lower-case in the look-up; if that cache is used, then it would just fall-out. either way, but please add a comment in each of the places I raised a question since the assumptions are currently unclear and error-prone. http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@386 PS4, Line 386: grp.getgrnam( > It's to get a group name. oh... its from the import. http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@388 PS4, Line 388: "invalidate metadata" > The issue is with role names having different cases and not with the privil sorry, meant role. this is an e2e test so if could be done here? anyways, if its not possible or extremely difficult, pls add a comment/perhaps a todo. this seems a bit circuitous to debug a case issue. -- To view, visit http://gerrit.cloudera.org:8080/11734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 16:32:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11734 ) Change subject: IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/11734/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11734/4//COMMIT_MSG@10 PS4, Line 10: original input role names I saw CatalogObjectCache is where users/roles are stored from AuthorizationPolicy. I see that the default, which is used, is to be case insensitive. So when it comes to names, they're lower-case in Impala. Issue is that it seems we sometimes use the name/key which is lowercase and sometimes the name from value (e.g., Principal) where case is preserved. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@172 PS4, Line 172: String so this is lowercase or can it have casing that does not match the name that's stored in Role/User? http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@185 PS4, Line 185: existingRole ... and this contains a name that is not lower-cased? http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@186 PS4, Line 186: sentryRole.getRoleName() afaict, this will get lowercased when performing the lookup. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@198 PS4, Line 198: getRoleName ... and this is lowercased? http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@386 PS4, Line 386: grp.getgrnam( what's this? http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@388 PS4, Line 388: "invalidate metadata" I think its good to have this test, but is there a more direct way to test this as well? For example, assign a privilege to a given role, then assign another privilege via sentry using different case. Will Impala see that the role then has both privs? -- To view, visit http://gerrit.cloudera.org:8080/11734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 07:06:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5004: Switch to sorting node for large TopN queries
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11698 ) Change subject: IMPALA-5004: Switch to sorting node for large TopN queries .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11698/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/11698/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@299 PS3, Line 299: // Add a Sort Node instead of a TopN Node if the estimated size of the materialized : // rows for TopN exceeds the value of TOPN_BYTES_LIMIT : long estimatedTopNMaterializedSize = 0; : f I'd prefer to put this block into SortInfo, called say, estimateMaterializedRecordSize. I see that SortInfo is up for debate, but at least we'll keep such sort-related code in one place. http://gerrit.cloudera.org:8080/#/c/11698/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@306 PS3, Line 306: estimatedTopNMaterializedSize += sortSlotDesc.getByteSize(); how's this compare with the row size estimate in SortNode.computeNodeResourceProfile? http://gerrit.cloudera.org:8080/#/c/11698/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@310 PS3, Line 310: *= I doubt we'll run into it in practice, but prefer to use PlanNode.checkedMultiply(..) here to deal with estimate overflows in a uniform way. -- To view, visit http://gerrit.cloudera.org:8080/11698 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34c9db33c9302b55e9978f53f9c7061f2806c8a9 Gerrit-Change-Number: 11698 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 19 Oct 2018 01:24:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7717: Handle concurrent partition changes in local catalog mode
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11732 ) Change subject: IMPALA-7717: Handle concurrent partition changes in local catalog mode .. Patch Set 3: Code-Review+2 (4 comments) looks, several small comments. http://gerrit.cloudera.org:8080/#/c/11732/3/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/11732/3/common/thrift/CatalogService.thrift@384 PS3, Line 384: 1. The partition that the partial fetch RPC is looking for has already been dropped. this is true of all the *_NOT_FOUND cases. bring this comment to the top of the enum? http://gerrit.cloudera.org:8080/#/c/11732/3/common/thrift/CatalogService.thrift@386 PS3, Line 386: can potentially remove (they do change, easily, often) http://gerrit.cloudera.org:8080/#/c/11732/3/common/thrift/CatalogService.thrift@387 PS3, Line 387: a remove http://gerrit.cloudera.org:8080/#/c/11732/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/11732/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1716 PS3, Line 1716: new TG good catch to not use "resp" since it may send back lots of stuff that will not be used. -- To view, visit http://gerrit.cloudera.org:8080/11732 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2aa103ee159ce9478af9b5b27b36bc0cc286f442 Gerrit-Change-Number: 11732 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Adrian Ng (389) Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 18 Oct 2018 23:46:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 ) Change subject: IMPALA-7597: wraps retries around Frontend metadata operations. .. Patch Set 8: Code-Review+2 test change broke another test. minor fix. core tests pass. carrying .+2 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 8 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 18 Oct 2018 20:27:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Hello Bharath Vissapragada, Tianyi Wang, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#8). Change subject: IMPALA-7597: wraps retries around Frontend metadata operations. .. IMPALA-7597: wraps retries around Frontend metadata operations. When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change wraps call-sites (primarily in Frontend.java) that acquire a Catalog, so have a chance of throwing an InconsistentMetadataFetchExecption. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 3 files changed, 289 insertions(+), 101 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/8 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 8 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11721 )
Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a
privilege
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@a214
PS4, Line 214:
what is the severity of the issue? ... priv doesn't get shown, crash, something
else?
http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@271
PS4, Line 271: Status TPrivilegeFromObjectName(const string& object_name,
TPrivilege* privilege) {
is there a comparable parsing method on the java side (frontend)? if so, would
be worthwhile for this to avoid duplication.
http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@274
PS4, Line 274: for (auto& s: split) {
const
--
To view, visit http://gerrit.cloudera.org:8080/11721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 18 Oct 2018 18:40:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Hello Bharath Vissapragada, Tianyi Wang, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#7). Change subject: IMPALA-7597: wraps retries around Frontend metadata operations. .. IMPALA-7597: wraps retries around Frontend metadata operations. When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change wraps call-sites (primarily in Frontend.java) that acquire a Catalog, so have a chance of throwing an InconsistentMetadataFetchExecption. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 3 files changed, 292 insertions(+), 101 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/7 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 7 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 ) Change subject: IMPALA-7597: wraps retries around Frontend metadata operations. .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/11608/5/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/11608/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1463 PS5, Line 1463: ImpalaInternalAdminUser.getInstance(); > add these cases to the test query list too? Done http://gerrit.cloudera.org:8080/#/c/11608/6/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11608/6/tests/custom_cluster/test_local_catalog.py@312 PS6, Line 312: def test_replan_on_stale_metadata(self, unique_database): > Port this test and below to use the helper method check..retries()? makes sense. they're looking for slightly different things (exception vs. retry). left a todo for now. -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 6 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 18 Oct 2018 06:36:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Hello Bharath Vissapragada, Tianyi Wang, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#6). Change subject: IMPALA-7597: wraps retries around Frontend metadata operations. .. IMPALA-7597: wraps retries around Frontend metadata operations. When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change wraps call-sites (primarily in Frontend.java) that acquire a Catalog, so have a chance of throwing an InconsistentMetadataFetchExecption. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 3 files changed, 290 insertions(+), 101 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/6 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 6 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11608 )
Change subject: IMPALA-7597: wraps retries around Frontend metadata operations.
..
Patch Set 5:
(5 comments)
thx for the suggestions. I brought up lambdas before but need java 8-- added a
todo to switch it over, which should make this part nicer.
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@681
PS4, Line 681: RI
> I meant, log the exception stack trace too. LOG.warn("foo", e) instead of L
Done
http://gerrit.cloudera.org:8080/#/c/11608/5/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11608/5/fe/src/main/java/org/apache/impala/service/Frontend.java@754
PS5, Line 754: List dbs = getCatalog().getDbs(matcher);
> mention no need to retry for this call path?
added a comment in the retry wrapper that explains when retries are not needed.
http://gerrit.cloudera.org:8080/#/c/11608/5/fe/src/main/java/org/apache/impala/service/Frontend.java@1463
PS5, Line 1463: FeTable table =
getCatalog().getTable(request.getTable_name().getDb_name(),
> this one?
yup, wrapped this one and another.
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py
File tests/custom_cluster/test_local_catalog.py:
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@280
PS4, Line 280: catalogd_args="--catalog_topic_mode=minimal")
> Ya, i meant to replace that "alter" with "refresh" which bumps the version
good suggestion. simpler now.
http://gerrit.cloudera.org:8080/#/c/11608/5/tests/custom_cluster/test_local_catalog.py
File tests/custom_cluster/test_local_catalog.py:
http://gerrit.cloudera.org:8080/#/c/11608/5/tests/custom_cluster/test_local_catalog.py@248
PS5, Line 248: attempt < 100
> Is this needed if we join for 30s?
I saw that without the patch, the failure comes up pretty quickly. With the
patch, there are lots of successes. The more queries run, the more memory was
taken up to the point of oom'ing the test runner. So I've capped the number of
queries but still guard using a timeout for the case where we're stuck in a
longer retry loop.
--
To view, visit http://gerrit.cloudera.org:8080/11608
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b
Gerrit-Change-Number: 11608
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 18 Oct 2018 05:43:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around Frontend metadata operations.
Hello Bharath Vissapragada, Tianyi Wang, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#5). Change subject: IMPALA-7597: wraps retries around Frontend metadata operations. .. IMPALA-7597: wraps retries around Frontend metadata operations. When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change wraps call-sites (primarily in Frontend.java) that acquire a Catalog, so have a chance of throwing an InconsistentMetadataFetchExecption. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 3 files changed, 264 insertions(+), 101 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/5 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 5 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11608 )
Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider
..
Patch Set 4:
(12 comments)
Agreed that spreading this retry pattern is not pretty. Some thoughts:
- is it an improvement when compared to the user seeing these exceptions?
- perhaps it indicates that there are too many service-level methods. I'm in
favor of funneling these through a common path, but I'd rather do that
restructure separately. an example of this is MetadataOp which fwict is used by
the hs2 entry point.
Other suggestions welcome.
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@660
PS4, Line 660:* Keeps track of retries when handling
InconsistentMetadataFetchExceptions.
> Maybe add more context and documentation. In which cases is this used etc.
Done
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@666
PS4, Line 666: private String msg_;
> final
Done
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@671
PS4, Line 671:
> ..of..
Done
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@680
PS4, Line 680: LOG.warn("Retried {}: {} (retry #{} of {})", msg_,
exception.getMessage(),
> Should any of this bubble up to the runtime profiles (if one exists) for th
would make sense to handle these uniformly with queries. added a todo. that
plumbling is better done separately.
http://gerrit.cloudera.org:8080/#/c/11608/4/fe/src/main/java/org/apache/impala/service/Frontend.java@681
PS4, Line 681: );
> , exception) to log the trace and you can probably skip exception.getMessag
wasn't entirely sure what's the suggestion here, but I was aiming for
consistency with the query-retry message
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py
File tests/custom_cluster/test_local_catalog.py:
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@173
PS4, Line 173: def test_cache_metrics(self, unique_database):
> did something change here or was it just moved to the top? Kind of confusin
separated these two types of tests into two classes so this test moved up here.
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@218
PS4, Line 218: _find_inconsistent_metadata
> name seems confusing. Maybe call check_metadata_fetch_retries or some such
renamed to _check_metadata_retries
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@221
PS4, Line 221: ,
> mention the randomness part?
Done
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@236
PS4, Line 236: def stress_thread(client):
> Add a comment on what this does?
Done
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@250
PS4, Line 250: inconsistent_seen[0] += 1
> catch and record other exceptions since this runs in a thread?
Done
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@258
PS4, Line 258: 5
> I doubt if this is enough, I noticed some concurrency errors required > 60s
I saw it repro at this level, but I upped in case its too close to the edge.
Set it to 30, which is the level used in tests already.
http://gerrit.cloudera.org:8080/#/c/11608/4/tests/custom_cluster/test_local_catalog.py@280
PS4, Line 280: "alter table %s.%s set tblproperties ('numRows'='3')" %
> Run these on 'functional' tables with a concurrent refresh? That way you ca
I didn't want to modify the state (see the alter on L280). That can leak to
other tests, which is a mess.
--
To view, visit http://gerrit.cloudera.org:8080/11608
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b
Gerrit-Change-Number: 11608
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 17 Oct 2018 17:36:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7689: Reduce per column per partition stats estimate size
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11706 ) Change subject: IMPALA-7689: Reduce per column per partition stats estimate size .. Patch Set 1: Code-Review+2 (1 comment) looks good. main thing for this one that would be helpful is to explain where the 400 came from (and from there, the 200). http://gerrit.cloudera.org:8080/#/c/11706/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/11706/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@199 PS1, Line 199: 200 the 400 byte estimate was pretty accurate from the examples I saw. is there a pointer you can put here to explain how that was derived, then explain the 50% of that from compression? -- To view, visit http://gerrit.cloudera.org:8080/11706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I347b41d9b298d7cd73ec812692172e0511415eee Gerrit-Change-Number: 11706 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 17 Oct 2018 03:42:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7669: Gracefully handle concurrent invalidate/partial fetch RPCs
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11638 ) Change subject: IMPALA-7669: Gracefully handle concurrent invalidate/partial fetch RPCs .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11638/3/tests/custom_cluster/test_local_catalog.py File tests/custom_cluster/test_local_catalog.py: http://gerrit.cloudera.org:8080/#/c/11638/3/tests/custom_cluster/test_local_catalog.py@274 PS3, Line 274: not failed_queries.empty(): : assert Fal can simplify to: assert not failed_queries.empty(), "..." -- To view, visit http://gerrit.cloudera.org:8080/11638 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8533f73f25ca42a20f146ddfd95d4213add9b705 Gerrit-Change-Number: 11638 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 16 Oct 2018 23:58:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7639: Move concurrent UDF tests to a custom cluster test
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11701 ) Change subject: IMPALA-7639: Move concurrent UDF tests to a custom cluster test .. Patch Set 1: Code-Review+2 thanks! -- To view, visit http://gerrit.cloudera.org:8080/11701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f255823167a4dd807a07276f630ef02435900a3 Gerrit-Change-Number: 11701 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 16 Oct 2018 22:08:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7669: Gracefully handle concurrent invalidate/partial fetch RPCs
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11638 ) Change subject: IMPALA-7669: Gracefully handle concurrent invalidate/partial fetch RPCs .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11638/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11638/2//COMMIT_MSG@22 PS2, Line 22: tight loop in parallel shells worthwhile making an e2e test for this? we have a couple of these for query re-planning. http://gerrit.cloudera.org:8080/#/c/11638/2/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java File fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java: http://gerrit.cloudera.org:8080/#/c/11638/2/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java@145 PS2, Line 145: cause_ == null This could be guarded equivalently at the call-site with isLoaded(). Here is an example where isLoaded can be either true or false. Default impl is that its always loaded but its unclear if all impls need to (or will need to) behave the same way. If partitions are fetched from a table that's not loaded, seems like a precondition violation. Not too strong an opinion here... putting it here makes all this easier to use and less error prone but at the risk of not uniformly handling the Table api more generally. http://gerrit.cloudera.org:8080/#/c/11638/2/fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java@149 PS2, Line 149: more meaningful lookup status perhaps TABLE_NOT_LOADED? -- To view, visit http://gerrit.cloudera.org:8080/11638 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8533f73f25ca42a20f146ddfd95d4213add9b705 Gerrit-Change-Number: 11638 Gerrit-PatchSet: 2 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 16 Oct 2018 07:24:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7702: enable fetch incremental stats by default
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11677 ) Change subject: IMPALA-7702: enable fetch incremental stats by default .. Patch Set 3: Code-Review+2 carry +2 -- To view, visit http://gerrit.cloudera.org:8080/11677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 Gerrit-Change-Number: 11677 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 16 Oct 2018 06:11:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7702: enable fetch incremental stats by default
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11677 to look at the new patch set (#3). Change subject: IMPALA-7702: enable fetch incremental stats by default .. IMPALA-7702: enable fetch incremental stats by default Flips the default from always off to always on for --pull_incremental_statistics. With this setting, the default is for coordinators to fetch incremental stats from catalogd directly (only when computing incremental stats) instead of receiving it from the statestore broadcast. Fetching incremental stats is not applicable when using a CatalogMetaProvider. By making fetch the default, it would require that --pull_incremental_statistics is set to false when enabling CatalogMetaProvider. This change makes --use_local_catalog to take priority over --pull_incremental_statistics so that when both are turned on, only the local catalog setting is enabled. Testing: - manual testing - moved the testing for pull incremental stats out of custom cluster tests since the default flipped - added tests that run with local catalog and pulling incremental stats. Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 --- M be/src/common/global-flags.cc M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java A tests/custom_cluster/test_incremental_stats.py D tests/custom_cluster/test_pull_stats.py M tests/metadata/test_compute_stats.py 5 files changed, 102 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11677/3 -- To view, visit http://gerrit.cloudera.org:8080/11677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 Gerrit-Change-Number: 11677 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7702: enable fetch incremental stats by default
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11677 ) Change subject: IMPALA-7702: enable fetch incremental stats by default .. Patch Set 2: Code-Review+2 (3 comments) carry +2 http://gerrit.cloudera.org:8080/#/c/11677/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11677/2//COMMIT_MSG@18 PS2, Line 18: This patch makes this : easier by not requiring this > rephrase? Done http://gerrit.cloudera.org:8080/#/c/11677/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11677/2/be/src/common/global-flags.cc@217 PS2, Line 217: "coordinators."); > Mention that this is not used in local catalog mode? Done http://gerrit.cloudera.org:8080/#/c/11677/2/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java: http://gerrit.cloudera.org:8080/#/c/11677/2/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@617 PS2, Line 617: > Add a comment that the pull flag doesn't make much sense in local catalog m Done -- To view, visit http://gerrit.cloudera.org:8080/11677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 Gerrit-Change-Number: 11677 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 16 Oct 2018 06:09:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7708: Switch to faster deflater compression level for incr stats
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11685 ) Change subject: IMPALA-7708: Switch to faster deflater compression level for incr stats .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11685 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife688aca3aed0e1e8af26c8348b850175d84b4ad Gerrit-Change-Number: 11685 Gerrit-PatchSet: 2 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Mon, 15 Oct 2018 20:24:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7702: enable fetch incremental stats by default
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11677 to look at the new patch set (#2). Change subject: IMPALA-7702: enable fetch incremental stats by default .. IMPALA-7702: enable fetch incremental stats by default Flips the default from always off to always on for --pull_incremental_statistics. With this setting, the default is for coordinators to fetch incremental stats from catalogd directly (only when computing incremental stats) instead of receiving it from the statestore broadcast. Fetching incremental stats is not applicable when using a CatalogMetaProvider. By making fetch the default, it would require that --pull_incremental_statistics is set to false when enabling CatalogMetaProvider. This patch makes this easier by not requiring this. Instead, if --use_local_catalog is set to true, that takes priority over --pull_incremental_statistics. Testing: - manual testing - moved the testing for pull incremental stats out of custom cluster tests since the default flipped - added tests that run with local catalog and pulling incremental stats. Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 --- M be/src/common/global-flags.cc M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java A tests/custom_cluster/test_incremental_stats.py D tests/custom_cluster/test_pull_stats.py M tests/metadata/test_compute_stats.py 5 files changed, 97 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11677/2 -- To view, visit http://gerrit.cloudera.org:8080/11677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 Gerrit-Change-Number: 11677 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7702: enable fetch incremental stats by default
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11677 Change subject: IMPALA-7702: enable fetch incremental stats by default .. IMPALA-7702: enable fetch incremental stats by default Flips the default from always off to always on for --pull_incremental_statistics. With this setting, the default is for coordinators to fetch incremental stats from catalogd directly (only when computing incremental stats) instead of receiving it from the statestore broadcast. Fetching incremental stats is not applicable when using a CatalogMetaProvider. By making fetch the default, it would require that --pull_incremental_statistics is set to false when enabling CatalogMetaProvider. This patch makes this easier by not requiring this. Instead, if --use_local_catalog is set to true, that takes priority over --pull_incremental_statistics. Testing: - manual testing Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 --- M be/src/common/global-flags.cc M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java 2 files changed, 2 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/11677/1 -- To view, visit http://gerrit.cloudera.org:8080/11677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5601a24f81bb3466cff5308c7093d2765bb1c481 Gerrit-Change-Number: 11677 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 ) Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. Patch Set 4: re-worked the tests a bit to separate out the retries from the other tests and to use less memory (was getting oom killed) -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 12 Oct 2018 22:38:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Hello Bharath Vissapragada, Tianyi Wang, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#4). Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. IMPALA-7597: wraps retries around CatalogMetaProvider When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change wraps call-sites (primarily in Frontend.java) that acquire a Catalog, so have a chance of throwing an InconsistentMetadataFetchExecption. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 3 files changed, 247 insertions(+), 100 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/4 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 4 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7622: adds profile metrics for incremental stats
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11670 ) Change subject: IMPALA-7622: adds profile metrics for incremental stats .. Patch Set 1: correct. -- To view, visit http://gerrit.cloudera.org:8080/11670 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94559a749500d44aa6aad564134d55c39e1d5273 Gerrit-Change-Number: 11670 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 12 Oct 2018 19:54:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7622: adds profile metrics for incremental stats
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11670 Change subject: IMPALA-7622: adds profile metrics for incremental stats .. IMPALA-7622: adds profile metrics for incremental stats Reapplies change after fixing where frontend profile is placed in runtime profile. When computing incremental statistics by fetching the stats directly from catalogd, a potentially expensive RPC is made from the impalad coordinator to catalogd. This change adds metrics to the frontend section of the profile to track how long the request takes, the size of the compressed bytes received, and the number of partitions received. The profile for a 'compute incremental ...' command on a table with no statistics looks like this: Frontend: - StatsFetch.CompressedBytes: 0 - StatsFetch.TotalPartitions: 24 - StatsFetch.NumPartitionsWithStats: 0 - StatsFetch.Time: 26ms And the profile looks as follows when the table has stats, so the stats are fetched: Frontend: - StatsFetch.CompressedBytes: 24622 - StatsFetch.TotalPartitions: 23 - StatsFetch.NumPartitionsWithStats: 23 - StatsFetch.Time: 14ms Testing: - manual inspection - e2e test to check the profile Change-Id: I94559a749500d44aa6aad564134d55c39e1d5273 --- M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_pull_stats.py 3 files changed, 93 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/11670/1 -- To view, visit http://gerrit.cloudera.org:8080/11670 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I94559a749500d44aa6aad564134d55c39e1d5273 Gerrit-Change-Number: 11670 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7701: grant option in SHOW GRANT always returns NULL from HS2 clients
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11663 ) Change subject: IMPALA-7701: grant_option in SHOW GRANT always returns NULL from HS2 clients .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e175544172b63d36dceedc61e1f47e0f910d7cf Gerrit-Change-Number: 11663 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 12 Oct 2018 16:22:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7701: grant option in SHOW GRANT always returns NULL from HS2 clients
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11663 ) Change subject: IMPALA-7701: grant_option in SHOW GRANT always returns NULL from HS2 clients .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11663/1/tests/authorization/test_show_grant.py File tests/authorization/test_show_grant.py: http://gerrit.cloudera.org:8080/#/c/11663/1/tests/authorization/test_show_grant.py@35 PS1, Line 35: class TestShowGrant(CustomClusterTestSuite): > A file name change from TestShowGrantUser to TestShowGrant since I think it that's fine, but since its mostly a file rename fwict, with a test added, it would be useful to hint to git that its a rename (not sure when this happens on its own vs. when it needs to be explicitly hinted) -- To view, visit http://gerrit.cloudera.org:8080/11663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e175544172b63d36dceedc61e1f47e0f910d7cf Gerrit-Change-Number: 11663 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 12 Oct 2018 15:51:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 ) Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. Patch Set 3: latest patch wraps retries higher-up at the callsites. -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 12 Oct 2018 04:31:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Hello Bharath Vissapragada, Tianyi Wang, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#3). Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. IMPALA-7597: wraps retries around CatalogMetaProvider When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change wraps call-sites (primarily in Frontend.java) that acquire a Catalog, so have a chance of throwing an InconsistentMetadataFetchExecption. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 3 files changed, 181 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/3 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 04:49:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11649/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11649/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2892 PS6, Line 2892: whole method : // redundant. didn't follow this part.. what do you mean by redundant? do you mean its "best-effort" here so if an exception happens, that's ok? -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 01:05:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 ) Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11608/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11608/2//COMMIT_MSG@18 PS2, Line 18: methods of CatalogMetaProvider with retries/logging. When obtaining > I briefly looked into making it a checked exception back when I initially c makes sense. also, its an exception that for now, is coming from one flavor of MetaProvider so unclear about insisting on it for all. -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 09 Oct 2018 22:30:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11608 ) Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11608/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11608/2//COMMIT_MSG@18 PS2, Line 18: methods of CatalogMetaProvider with retries/logging. When obtaining > good point. most of the top-level "describe" methods use strings as args, n I'll look into wrapping the frontend call-sites, so similar handling as with the createExecRequest code path. I thought I saw some discussion about making this a checked exception, but otherwise, if there are better, less cumbersome ways to deal with this, let me know. -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 09 Oct 2018 22:21:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11608 )
Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11608/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11608/2//COMMIT_MSG@18
PS2, Line 18: methods of CatalogMetaProvider with retries/logging. When
obtaining
> I'm confused how this is supposed to work. Don't we need to retry from the
good point. most of the top-level "describe" methods use strings as args, not
refs. the refs are mostly used for everything under createExecRequest, and
there, we're guarding at the top-level. the method to get column stats and the
one that looks up all partition info should change as a result. will confirm
with tests.
http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java
File
fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java:
http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java@126
PS1, Line 126: while (true) {
: try {
: return provider_.loadTableNames(dbName);
: } catch (InconsistentMetadataFetchException e) {
: tracker.handleRetryOrThrow(e);
: }
> can we factor out while(true) try {} catch {retry} into a helper? I think
will try it but only after we're settled on this approach.
http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/11608/1/fe/src/main/java/org/apache/impala/service/Frontend.java@834
PS1, Line 834: ribeResult
> It looks like the callers should now understand whether an operation should
As discussed, I was trading off looping too many times with retry (consider
getCatalogMetrics and all query paths under createExecRequest) vs. letting one
of these inconsistent metadata exceptions loose. We spend far more cycles in
the latter so opted to not go with an approach that changes the default to loop.
--
To view, visit http://gerrit.cloudera.org:8080/11608
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b
Gerrit-Change-Number: 11608
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 09 Oct 2018 20:48:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11608 to look at the new patch set (#2). Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. IMPALA-7597: wraps retries around CatalogMetaProvider When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change adds a RetryableMetaProvider that wraps the relevant methods of CatalogMetaProvider with retries/logging. When obtaining a Catalog from the FeCatalogManager, the caller can switch between obtaining a Catalog that does not retry and one that does. Several Frontend call-sites have been updated accordingly. For example, GetCatalogMetrics does not really care if an exception is thrown when fetching the number of tables so using a Catalog without retries is used (as before). However, when showing various metadata such as partitions or files, a retryable Catalog is now used. Note that when a local catalog is not used, the retryable catalog is a no-op. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- A fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 5 files changed, 328 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/2 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7597: wraps retries around CatalogMetaProvider
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11608 Change subject: IMPALA-7597: wraps retries around CatalogMetaProvider .. IMPALA-7597: wraps retries around CatalogMetaProvider When configured to use the local catalog, concurrent metadata reads and writes can cause the CatalogMetaProvider to throw an InconsistentMetadataFetchException. Queries have been wrapped with a retry loop, but the other frontend methods, such listing table or partition information, can also fail from the same error. These errors were seen under a workload consisting of concurrent adding and showing partitions. This change adds a RetryableMetaProvider that wraps the relevant methods of CatalogMetaProvider with retries/logging. When obtaining a Catalog from the FeCatalogManager, the caller can switch between obtaining a Catalog that does not retry and one that does. Several Frontend call-sites have been updated accordingly. For example, GetCatalogMetrics does not really care if an exception is thrown when fetching the number of tables so using a Catalog without retries is used (as before). However, when showing various metadata such as partitions or files, a retryable Catalog is now used. Note that when a local catalog is not used, the retryable catalog is a no-op. Testing: - added a test that checks whether inconsistent metadata exceptions are seen in a concurrent workload. Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b --- A fe/src/main/java/org/apache/impala/catalog/local/RetryableMetaProvider.java M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M tests/custom_cluster/test_local_catalog.py 5 files changed, 323 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/11608/1 -- To view, visit http://gerrit.cloudera.org:8080/11608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I43a21571d54a7966c5c68bea1fe69dbc62be2a0b Gerrit-Change-Number: 11608 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7626: Throttle catalog partial RPC requests
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11561 ) Change subject: IMPALA-7626: Throttle catalog partial RPC requests .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11f77a16cfa38ada42d8b7c859850198ea7dd142 Gerrit-Change-Number: 11561 Gerrit-PatchSet: 11 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Sat, 06 Oct 2018 01:09:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7671: Fix broken SHOW GRANT USER ON
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11598 ) Change subject: IMPALA-7671: Fix broken SHOW GRANT USER ON .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11598 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7adc403caddd18e5a954cf6affd5d1d555b9f5f0 Gerrit-Change-Number: 11598 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 05 Oct 2018 21:42:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7671: Fix broken SHOW GRANT USER ON
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11598 ) Change subject: IMPALA-7671: Fix broken SHOW GRANT USER ON .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11598/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11598/1//COMMIT_MSG@9 PS1, Line 9: broken what caused the broken test? -- To view, visit http://gerrit.cloudera.org:8080/11598 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7adc403caddd18e5a954cf6affd5d1d555b9f5f0 Gerrit-Change-Number: 11598 Gerrit-PatchSet: 1 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 05 Oct 2018 21:05:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6323 Allow constant "partition by" expressions.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11556 )
Change subject: IMPALA-6323 Allow constant "partition by" expressions.
..
Patch Set 2:
(1 comment)
lgtm, just a minor style comment.
http://gerrit.cloudera.org:8080/#/c/11556/2/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java:
http://gerrit.cloudera.org:8080/#/c/11556/2/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@91
PS2, Line 91: {
remove the braces (project convention).
--
To view, visit http://gerrit.cloudera.org:8080/11556
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idf129026c45120e9470df601268863634037908c
Gerrit-Change-Number: 11556
Gerrit-PatchSet: 2
Gerrit-Owner: Michal Ostrowski
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 05 Oct 2018 17:30:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7633: count user privilege isn't 0 at the end of test owner
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11595 ) Change subject: IMPALA-7633: count_user_privilege isn't 0 at the end of test_owner .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11595 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifbba0fbd0e24a24b3f2af82ad5209f3fb7fb387b Gerrit-Change-Number: 11595 Gerrit-PatchSet: 1 Gerrit-Owner: Adam Holley Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 05 Oct 2018 17:00:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7626: Throttle catalog partial RPC requests
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11561 ) Change subject: IMPALA-7626: Throttle catalog partial RPC requests .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11f77a16cfa38ada42d8b7c859850198ea7dd142 Gerrit-Change-Number: 11561 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 05 Oct 2018 02:12:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7484: Do not interpret unrecognized hints as straight join hints.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Do not interpret unrecognized hints as straight_join hints. .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 9 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 05 Oct 2018 02:12:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7626: Throttle catalog partial RPC requests
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11561 ) Change subject: IMPALA-7626: Throttle catalog partial RPC requests .. Patch Set 7: (3 comments) lgtm. question about a possible race. perhaps its not an issue. http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2115 PS7, Line 2115:* A wrapper around doGetPartialCatalogObject() that controls the number concurrent nit: of http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2119 PS7, Line 2119: ( nit: weird formatting http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2131 PS7, Line 2131: // this method. is there a chance that we get interrupted right on this line and then miss the release in the finally clause? -- To view, visit http://gerrit.cloudera.org:8080/11561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11f77a16cfa38ada42d8b7c859850198ea7dd142 Gerrit-Change-Number: 11561 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Fri, 05 Oct 2018 01:29:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7626: Throttle catalog partial RPC requests
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11561 ) Change subject: IMPALA-7626: Throttle catalog partial RPC requests .. Patch Set 6: (5 comments) lgtm. mostly nits/stale messages. http://gerrit.cloudera.org:8080/#/c/11561/6/be/src/catalog/catalog-server.h File be/src/catalog/catalog-server.h: http://gerrit.cloudera.org:8080/#/c/11561/6/be/src/catalog/catalog-server.h@94 PS6, Line 94: partial_rpc_call rpc_call has a bit of redundancy. I also read this as "partial_rpc", which looks odd. how about partial_fetch_rpc_queue_len_metric_? http://gerrit.cloudera.org:8080/#/c/11561/6/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/11561/6/be/src/catalog/catalog-server.cc@382 PS6, Line 382: while(1) nit: while (true) seems to be more commonly used here. http://gerrit.cloudera.org:8080/#/c/11561/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11561/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@247 PS6, Line 247: catalog_partial_rpc_max_parallel_runs stale: catalog_max_parallel_partial_rpc http://gerrit.cloudera.org:8080/#/c/11561/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2119 PS6, Line 2119: nit: extra line http://gerrit.cloudera.org:8080/#/c/11561/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2130 PS6, Line 2130: catalog_partial_rpc_max_parallel_runs stale -- To view, visit http://gerrit.cloudera.org:8080/11561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11f77a16cfa38ada42d8b7c859850198ea7dd142 Gerrit-Change-Number: 11561 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 04 Oct 2018 05:06:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Do not interpret unrecognized hints as straight join hints.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Do not interpret unrecognized hints as straight_join hints. .. Patch Set 6: (2 comments) lgtm, couple of formatting issues. not sure why the checks didn't complain about these. http://gerrit.cloudera.org:8080/#/c/11568/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1825 PS6, Line 1825: /** indentation is off http://gerrit.cloudera.org:8080/#/c/11568/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1841 PS6, Line 1841: "select %sstraight_join%s * from functional.alltypes", prefix, suffix),null)); add a space (and several more below) -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 6 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 04 Oct 2018 00:15:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Do not interpret unrecognized hints as straight join hints.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/11568 )
Change subject: IMPALA-7484: Do not interpret unrecognized hints as
straight_join hints.
..
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11568/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:
http://gerrit.cloudera.org:8080/#/c/11568/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1830
PS5, Line 1830: Anal
I was expecting all of these cases to use the wrapper. We'll get more coverage
this way. The error addressed here would not have been present if these checks
were in place.
http://gerrit.cloudera.org:8080/#/c/11568/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1852
PS5, Line 1852: er to test if straight_join hints
pls reword this so its clear that this does not test the hint (just returns a
state)
http://gerrit.cloudera.org:8080/#/c/11568/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1855
PS5, Line 1855: check
replace "check" with "has"
http://gerrit.cloudera.org:8080/#/c/11568/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1855
PS5, Line 1855: private boolean checkStraightJoin(String stmt, String
expectedWarning){
put the helper before its use, e.g., on L1824
http://gerrit.cloudera.org:8080/#/c/11568/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1858
PS5, Line 1858: // Unrecognized hint
what's this comment for?
--
To view, visit http://gerrit.cloudera.org:8080/11568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04
Gerrit-Change-Number: 11568
Gerrit-PatchSet: 5
Gerrit-Owner: Anurag Mantripragada
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 03 Oct 2018 23:06:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7527: add fetch-from-catalogd cache info to profile
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11569 ) Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile .. Patch Set 3: Code-Review+2 carry +2 -- To view, visit http://gerrit.cloudera.org:8080/11569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419be157168cddb7521ea61e8f86733306b9315e Gerrit-Change-Number: 11569 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 17:59:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7527: add fetch-from-catalogd cache info to profile
Hello Bharath Vissapragada, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11569 to look at the new patch set (#3). Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile .. IMPALA-7527: add fetch-from-catalogd cache info to profile Reapplies reverted IMPALA-7527. Adding a top-level entry to the profile broke downstream consumers. The change here is to add the additional stats to the summary profile. This patch adds a Java wrapper for a RuntimeProfile object. The wrapper supports some basic operations like non-hierarchical counters and informational strings. During planning, a profile is created, and passed back to the backend as part of the ExecRequest. The backend then updates the query profile based on the info emitted from the frontend. This patch also adds the first use case for this profile information: the CatalogdMetaProvider emits counters for cache hits, misses, and fetch times, broken down by metadata category. The emitted profile is a bit of a superset of the existing 'timeline' functionality. However, it seems that some tools may parse the timeline in its current location in the profile, so moving it might be incompatible. I elected to leave that alone for now and just emit counters in the new profile. Change-Id: I419be157168cddb7521ea61e8f86733306b9315e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/util/runtime-profile.h M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java M fe/src/main/java/org/apache/impala/service/Frontend.java A fe/src/main/java/org/apache/impala/service/FrontendProfile.java M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java M tests/custom_cluster/test_local_catalog.py 10 files changed, 301 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/11569/3 -- To view, visit http://gerrit.cloudera.org:8080/11569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I419be157168cddb7521ea61e8f86733306b9315e Gerrit-Change-Number: 11569 Gerrit-PatchSet: 3 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7527: add fetch-from-catalogd cache info to profile
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11569 ) Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile .. Patch Set 2: Code-Review+2 (1 comment) carry +2 http://gerrit.cloudera.org:8080/#/c/11569/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11569/1//COMMIT_MSG@10 PS1, Line 10: broke > Ok. May be comment it in the header file that we intentionally put it as a Done -- To view, visit http://gerrit.cloudera.org:8080/11569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419be157168cddb7521ea61e8f86733306b9315e Gerrit-Change-Number: 11569 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 17:49:27 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7527: add fetch-from-catalogd cache info to profile
Hello Bharath Vissapragada, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11569 to look at the new patch set (#2). Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile .. IMPALA-7527: add fetch-from-catalogd cache info to profile Reapplies reverted IMPALA-7527. Adding a top-level entry to the profile broke downstream consumers. The change here is to add the additional stats to the summary profile. This patch adds a Java wrapper for a RuntimeProfile object. The wrapper supports some basic operations like non-hierarchical counters and informational strings. During planning, a profile is created, and passed back to the backend as part of the ExecRequest. The backend then updates the query profile based on the info emitted from the frontend. This patch also adds the first use case for this profile information: the CatalogdMetaProvider emits counters for cache hits, misses, and fetch times, broken down by metadata category. The emitted profile is a bit of a superset of the existing 'timeline' functionality. However, it seems that some tools may parse the timeline in its current location in the profile, so moving it might be incompatible. I elected to leave that alone for now and just emit counters in the new profile. Change-Id: I419be157168cddb7521ea61e8f86733306b9315e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/util/runtime-profile.h M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java M fe/src/main/java/org/apache/impala/service/Frontend.java A fe/src/main/java/org/apache/impala/service/FrontendProfile.java M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java M tests/custom_cluster/test_local_catalog.py 10 files changed, 301 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/11569/2 -- To view, visit http://gerrit.cloudera.org:8080/11569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I419be157168cddb7521ea61e8f86733306b9315e Gerrit-Change-Number: 11569 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1849 PS4, Line 1849: assertTrue(ctx.getAnalyzer().isStraightJoin()); An improvement to this collection of tests would be to wrap the pattern of ctx; AnalyzeOk; assertTrue|False in a helper method, say expectStraightJoin(query, warning?, true| false). That way, each of these asserts that the query analyzes, checks a warning message if applicable, and states whether the hint was applied. Also, is the test on L1841 testing the same thing as the new one on L1852? -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 07:28:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only i .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-7484: Unrecognized hints are interpreted as straight_join add a newline http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@8 PS3, Line 8: Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only if the hintis a straight_join. wrap this appropriately. http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@10 PS3, Line 10: Testing: Added two test cases to TestSelectListHints. 1) To make assert straight_join is not set when hint is unrecognized and 2) To assert it is properly set when hint is indeed a straigh_join. same here, please wrap the lines. http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1845 PS3, Line 1845: //if hint is straight join add a space after '//' http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1851 PS3, Line 1851: //Unrecognized hint same here, add a space. -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 05:50:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-7626: Throttle catalog partial RPC requests
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11561 ) Change subject: WIP IMPALA-7626: Throttle catalog partial RPC requests .. Patch Set 4: (7 comments) for testing, depending on that new metric might be racy. probably easiest to do this via the junit tests. perhaps you can make that "do..." method protected and override it with a sleep to make it easier to check the average queue len over a longer duration. http://gerrit.cloudera.org:8080/#/c/11561/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11561/4//COMMIT_MSG@16 PS4, Line 16: are blocked until the current requests are serviced. Adds the following or they timeout http://gerrit.cloudera.org:8080/#/c/11561/4/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/11561/4/be/src/catalog/catalog-server.cc@a237 PS4, Line 237: we lost this message-- was it useful? what's shown instead? http://gerrit.cloudera.org:8080/#/c/11561/4/be/src/catalog/catalog-server.cc@71 PS4, Line 71: Should Must ... I think there's even a precondition that checks it. http://gerrit.cloudera.org:8080/#/c/11561/4/be/src/catalog/catalog-server.cc@71 PS4, Line 71: for access to the resources nit: to run http://gerrit.cloudera.org:8080/#/c/11561/4/common/thrift/BackendGflags.thrift File common/thrift/BackendGflags.thrift: http://gerrit.cloudera.org:8080/#/c/11561/4/common/thrift/BackendGflags.thrift@103 PS4, Line 103: parallel_runs parallel_runs sounds odd. perhaps max_catalog_parallel_partial_rpc ? http://gerrit.cloudera.org:8080/#/c/11561/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11561/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2113 PS4, Line 2113: public TGetPartialCatalogObjectResponse getPartialCatalogObject method doc http://gerrit.cloudera.org:8080/#/c/11561/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2118 PS4, Line 2118: permit ? -- To view, visit http://gerrit.cloudera.org:8080/11561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11f77a16cfa38ada42d8b7c859850198ea7dd142 Gerrit-Change-Number: 11561 Gerrit-PatchSet: 4 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 00:12:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7527: add fetch-from-catalogd cache info to profile
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11569 ) Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile .. Patch Set 1: (1 comment) The change is to put these metrics in the summary profile, not the top-level profile. Let me know if that should be further clarified in the commit message. http://gerrit.cloudera.org:8080/#/c/11569/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11569/1//COMMIT_MSG@10 PS1, Line 10: broke > Can we add some tests in Impala for this to avoid this in the future? little hesitant to commit that profile ordering as a contract at this point. -- To view, visit http://gerrit.cloudera.org:8080/11569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419be157168cddb7521ea61e8f86733306b9315e Gerrit-Change-Number: 11569 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 02 Oct 2018 23:54:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7527: add fetch-from-catalogd cache info to profile
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11569 Change subject: IMPALA-7527: add fetch-from-catalogd cache info to profile .. IMPALA-7527: add fetch-from-catalogd cache info to profile Reapplies reverted IMPALA-7527. Adding a top-level entry to the profile broke downstream consumers. The change here is to add the additional stats to the summary profile. This patch adds a Java wrapper for a RuntimeProfile object. The wrapper supports some basic operations like non-hierarchical counters and informational strings. During planning, a profile is created, and passed back to the backend as part of the ExecRequest. The backend then updates the query profile based on the info emitted from the frontend. This patch also adds the first use case for this profile information: the CatalogdMetaProvider emits counters for cache hits, misses, and fetch times, broken down by metadata category. The emitted profile is a bit of a superset of the existing 'timeline' functionality. However, it seems that some tools may parse the timeline in its current location in the profile, so moving it might be incompatible. I elected to leave that alone for now and just emit counters in the new profile. Change-Id: I419be157168cddb7521ea61e8f86733306b9315e --- M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/util/runtime-profile.h M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java M fe/src/main/java/org/apache/impala/service/Frontend.java A fe/src/main/java/org/apache/impala/service/FrontendProfile.java M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java M tests/custom_cluster/test_local_catalog.py 10 files changed, 300 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/11569/1 -- To view, visit http://gerrit.cloudera.org:8080/11569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I419be157168cddb7521ea61e8f86733306b9315e Gerrit-Change-Number: 11569 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac Gerrit-Reviewer: Todd Lipcon
