[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Manually checked thread dump for proper thread naming, OOM exception, runtime exception and too slow and too large request warning. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Reviewed-on: http://gerrit.cloudera.org:8080/19198 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/common/JniUtil.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 4 files changed, 419 insertions(+), 286 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 10 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 18:00:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8941/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 12:52:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 9: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8938/ -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 10:39:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 8: Code-Review+2 LGTM. Carry Csaba's +1. Thanks for making this change! -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 05:27:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 05:28:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8938/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 03 Jan 2023 05:28:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/12078/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 02 Jan 2023 13:08:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has posted comments on this change. (
http://gerrit.cloudera.org:8080/19198 )
Change subject: IMPALA-11478: Cleanup JniCatalog
..
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/common/JniUtil.java
File fe/src/main/java/org/apache/impala/common/JniUtil.java:
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/common/JniUtil.java@146
PS7, Line 146: public static class OperationLog {
: private final long startTime;
: p
> nit: could you move these comment to line 174?
Done
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@410
PS7, Line 410:
> typo: prioritize
Done
--
To view, visit http://gerrit.cloudera.org:8080/19198
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab
Gerrit-Change-Number: 19198
Gerrit-PatchSet: 8
Gerrit-Owner: Peter Rozsa
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Mon, 02 Jan 2023 12:47:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19198 to look at the new patch set (#8). Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Manually checked thread dump for proper thread naming, OOM exception, runtime exception and too slow and too large request warning. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/common/JniUtil.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 4 files changed, 419 insertions(+), 286 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/8 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Quanlong Huang has posted comments on this change. (
http://gerrit.cloudera.org:8080/19198 )
Change subject: IMPALA-11478: Cleanup JniCatalog
..
Patch Set 7: Code-Review+1
(3 comments)
I have some more comments on the 'shortDesc' strings. It'd be nice to add the
operation targets so whenever we take a jstack we know the tables that each
JniCatalog thread is working on.
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/common/JniUtil.java
File fe/src/main/java/org/apache/impala/common/JniUtil.java:
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/common/JniUtil.java@146
PS7, Line 146: /**
:* Warn if the result size or the response time exceeds
thresholds.
:*/
nit: could you move these comment to line 174?
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@410
PS7, Line 410: priorize
typo: prioritize
Can we also add the table list in 'shortDesc'? i.e.
String shortDesc = "prioritize load " +
request.getObject_descs().stream()
.map(TCatalogObject::getTable)
.map(t -> t.getDb_name() + "." + t.getTbl_name())
.collect(Collectors.joining(", "));
http://gerrit.cloudera.org:8080/#/c/19198/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@478
PS7, Line 478: String shortDesc = "update table usage";
It'd be nice to add the table list here as well, i.e.
String shortDesc = "update table usage " +
thriftReq.getUsages().stream()
.map(TTableUsage::getTable_name)
.map(TableName::thriftToString)
.collect(Collectors.joining(", "));
--
To view, visit http://gerrit.cloudera.org:8080/19198
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab
Gerrit-Change-Number: 19198
Gerrit-PatchSet: 7
Gerrit-Owner: Peter Rozsa
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Mon, 12 Dec 2022 03:33:21 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/11989/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 07 Dec 2022 13:03:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/19198/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/19198/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java@230 PS6, Line 230: execAndSe > naming: maybe something like execAndSerialize would be clearer? Done -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 07 Dec 2022 12:45:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19198 to look at the new patch set (#7). Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Manually checked thread dump for proper thread naming, OOM exception, runtime exception and too slow and too large request warning. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/common/JniUtil.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 4 files changed, 398 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/7 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 6: Code-Review+1 (1 comment) Thanks for making it even more compact! http://gerrit.cloudera.org:8080/#/c/19198/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/19198/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java@230 PS6, Line 230: serialize naming: maybe something like execAndSerialize would be clearer? -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 07 Dec 2022 12:11:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@20 PS4, Line 20: import com.google.common.annotations.VisibleForTesting; > Using a static function from other class without prefixing with the class n Done http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@252 PS4, Line 252: params.getNative_catalog_server_ptr(), params.getFrom_version()); : return new TGetCatalogDeltaResponse(catalog > This looks like a very common pattern, an alternative of execOp could avoid A specialized method called serialize added for this purpose http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@265 PS4, Line 265: return serialize( > If already cleaning up this could get a trailing _ (it confused me for a bi Done -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 07 Dec 2022 10:31:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/11986/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 07 Dec 2022 10:23:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19198 to look at the new patch set (#6). Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Manually checked thread dump for proper thread naming, OOM exception, runtime exception and too slow and too large request warning. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/common/JniUtil.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 4 files changed, 404 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/6 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/19198 )
Change subject: IMPALA-11478: Cleanup JniCatalog
..
Patch Set 5: Code-Review+1
(3 comments)
Looks good to me, just some extra cleanup / redundancy reduction ideas.
http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@20
PS4, Line 20: import static org.apache.impala.service.JniCatalogOp.execOp;
Using a static function from other class without prefixing with the class name
looks a bit unusual for me. A simple private wrapper function could be created
in JniCatalog that calls to JniCatalogOp.execOp() if the goal is the make the
code shorter by avoiding adding JniCatalog. at every call.
Having such wrapper member function could also reach members like
protocolFactory_ if needed.
http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@252
PS4, Line 252: byte[] res =
serializer.serialize(catalogOpExecutor_.execDdlRequest(params));
: return Pair.create(res, (long) res.length);
This looks like a very common pattern, an alternative of execOp could avoid the
boilerplate code by expecting a TSerializer an extra param while the lambda
would return TSerializable. So this could look like:
return execOp2("execDdl", shortDesc, () -> {
return catalogOpExecutor_.execDdlRequest(params));
}, params. serializer);
http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/service/JniCatalog.java@265
PS4, Line 265: catalogOperationUsage
If already cleaning up this could get a trailing _ (it confused me for a bit
first).
--
To view, visit http://gerrit.cloudera.org:8080/19198
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab
Gerrit-Change-Number: 19198
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Tue, 06 Dec 2022 15:13:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 5: Code-Review+1 LGTM. Give +1 first if other reviewers also want to take a look. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 01 Dec 2022 13:17:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/11932/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 01 Dec 2022 09:14:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG@13 PS3, Line 13: Frontend test added as JniCatalogOpTest. > Thanks for doing the experiment! These looks good to me. Just one question: If the duration is 0 then PrintUtils.printTimeMs() will return an empty string, that's why these log entries have no time spent value, and it worked the same way before this patch. http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/common/JniUtil.java@176 PS4, Line 176: getDurationFromStart(); > nit: getDurationFromStart() Done -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 01 Dec 2022 08:53:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19198 to look at the new patch set (#5). Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Manually checked thread dump for proper thread naming, OOM exception, runtime exception and too slow and too large request warning. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/common/JniUtil.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 4 files changed, 377 insertions(+), 251 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/5 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 4: (2 comments) The refactor is pretty good! I just have minor comments. http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG@13 PS3, Line 13: Frontend test added as JniCatalogOpTest. > I checked manually the exceptions and the thread dump for getDbs and getCat Thanks for doing the experiment! These looks good to me. Just one question: "Time spent" in the last two logs don't have the actual duration. Is it missed by copy-paste mistake, or is it a bug? http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/19198/4/fe/src/main/java/org/apache/impala/common/JniUtil.java@176 PS4, Line 176: System.currentTimeMillis() - startTime nit: getDurationFromStart() -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 01 Dec 2022 02:12:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/11883/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Peter Rozsa Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Tue, 22 Nov 2022 11:18:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has posted comments on this change. (
http://gerrit.cloudera.org:8080/19198 )
Change subject: IMPALA-11478: Cleanup JniCatalog
..
Patch Set 4:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG@13
PS3, Line 13: Frontend test added as JniCatalogOpTest.
> We are lack of tests for thread annotations and logs. It'd be nice if you c
I checked manually the exceptions and the thread dump for getDbs and
getCatalogObject methods
Thread dump shows the correct name during calls:
Thread-18 [getting databases]" #62 prio=5 os_prio=0 tid=0x0dd77800
nid=0x24a19 runnable [0x7f8bba89b000]
I1122 10:36:07.223536 154715 JniUtil.java:161] getDbs request: getting databases
I1122 10:36:16.965569 154715 JniUtil.java:165] Finished getDbs request: getting
databases. Time spent: 9s742ms
Too slow and large error message remained the same (forced with debugger, so
the durations are off a bit):
W1122 10:52:32.269219 174059 JniUtil.java:188] Response too large and too slow:
size=7115 (6.95KB), duration=35392ms (35s392ms), method: getDbs, request:
TGetDbsParams()
W1122 10:55:05.165840 174059 JniUtil.java:188] Response too large and too slow:
size=7115 (6.95KB), duration=1404300ms (23m24s), method: getDbs, request:
TGetDbsParams()
OOM, Runtime exception logs are resembling with the previous solution:
E1122 11:36:28.131889 226354 JniUtil.java:171] Error in getting thrift catalog
object of TABLE:tpch_nested_orc_def.part. Time spent:
I1122 11:36:28.132845 226354 jni-util.cc:288] java.lang.OutOfMemoryError: aa
at
org.apache.impala.service.JniCatalog.lambda$getCatalogObject$7(JniCatalog.java:347)
at org.apache.impala.service.JniCatalogOp.execOp(JniCatalogOp.java:64)
at org.apache.impala.service.JniCatalogOp.execOp(JniCatalogOp.java:86)
at
org.apache.impala.service.JniCatalog.getCatalogObject(JniCatalog.java:344)
E1122 11:41:34.900446 232524 JniUtil.java:171] Error in getting thrift catalog
object of TABLE:tpch_nested_orc_def.part. Time spent:
I1122 11:41:34.900624 232524 jni-util.cc:288]
org.apache.impala.common.ImpalaRuntimeException: aa
at
org.apache.impala.service.JniCatalog.lambda$getCatalogObject$7(JniCatalog.java:348)
at org.apache.impala.service.JniCatalogOp.execOp(JniCatalogOp.java:64)
at org.apache.impala.service.JniCatalogOp.execOp(JniCatalogOp.java:86)
at
org.apache.impala.service.JniCatalog.getCatalogObject(JniCatalog.java:345)
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@20
PS3, Line 20: import static org.apache.impala.service.JniCatalogOp.execOp;
> Please avoid star import
Done
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@191
PS3, Line 191: v
> nit: remove DOT here
Done
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@294
PS3, Line 294: params
> Shouldn't this be "params"?
Done
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@316
PS3, Line 316: params
> params?
Done
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java
File fe/src/main/java/org/apache/impala/service/JniCatalogOp.java:
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java@20
PS3, Line 20: com.google.common.base.Preco
> We use "com.google.common.base.Preconditions" instead:
Done
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java@38
PS3, Line 38:
: /**
:* Executes @operand in an annotated thread environme
> Could you add some commends about the parameters? especially the return typ
Done
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java@57
PS3, Line 57: throws ImpalaException, TException {
: Preconditions.checkNotNull(operand);
: Preconditions.checkNotNull(finalClause);
> It'd be nice if move these into JniUtil#logResponse().
Moved logging logic to a separate class.
--
To view, visit http://gerrit.cloudera.org:8080/19198
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab
Gerrit-Change-Number: 19198
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Peter Rozsa
Gerrit-Reviewer:
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Manually checked thread dump for proper thread naming, OOM exception, runtime exception and too slow and too large request warning. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/common/JniUtil.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 4 files changed, 377 insertions(+), 251 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/4 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Quanlong Huang has posted comments on this change. (
http://gerrit.cloudera.org:8080/19198 )
Change subject: IMPALA-11478: Cleanup JniCatalog
..
Patch Set 3:
(9 comments)
This patch looks pretty good to me!
http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19198/3//COMMIT_MSG@13
PS3, Line 13: Frontend test added as JniCatalogOpTest.
We are lack of tests for thread annotations and logs. It'd be nice if you could
verify these as I've done in the previous patch:
https://gerrit.cloudera.org/c/18772/7//COMMIT_MSG#21
- Manually add codes to throw OutOfMemoryError and verify the logs shown as
expected.
- Run test_concurrent_ddls.py and metadata tests. Capture jstacks and verify
the thread annotations are shown.
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/common/JniUtil.java
File fe/src/main/java/org/apache/impala/common/JniUtil.java:
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/common/JniUtil.java@166
PS3, Line 166: public static void logResponse(long startTime, TBase
thriftReq, String method) {
: logResponse(0, startTime, thriftReq, method);
: }
:
: public static void logResponse(long startTime, String method) {
: logResponse(startTime, null, method);
: }
We can remove these if they are no longer used.
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@20
PS3, Line 20: import static org.apache.impala.service.JniCatalogOp.*;
Please avoid star import
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@191
PS3, Line 191: .
nit: remove DOT here
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@294
PS3, Line 294: result
Shouldn't this be "params"?
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalog.java@316
PS3, Line 316: result
params?
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java
File fe/src/main/java/org/apache/impala/service/JniCatalogOp.java:
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java@20
PS3, Line 20: jline.internal.Preconditions
We use "com.google.common.base.Preconditions" instead:
https://github.com/apache/impala/blob/97a506c6560e7a0c6c5ba7d098f07c6b04ef6cad/fe/src/main/java/org/apache/impala/service/JniCatalog.java#L21
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java@38
PS3, Line 38: public static > RESULT
execOp(String methodName,
: String shortDescription, JniCatalogOpCallable> operand,
: PARAMETER requestParameter, Runnable finalClause)
Could you add some commends about the parameters? especially the return type of
"operand"
http://gerrit.cloudera.org:8080/#/c/19198/3/fe/src/main/java/org/apache/impala/service/JniCatalogOp.java@57
PS3, Line 57: long duration = System.currentTimeMillis() - start;
: LOG.info("Finished {} request: {}. Time spent: {}",
methodName, shortDescription,
: PrintUtils.printTimeMs(duration));
It'd be nice if move these into JniUtil#logResponse().
--
To view, visit http://gerrit.cloudera.org:8080/19198
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab
Gerrit-Change-Number: 19198
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Rozsa
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Quanlong Huang
Gerrit-Comment-Date: Mon, 14 Nov 2022 09:08:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/11831/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 10 Nov 2022 17:02:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19198 to look at the new patch set (#3). Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 3 files changed, 315 insertions(+), 231 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/3 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19198 ) Change subject: IMPALA-11478: Cleanup JniCatalog .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/11828/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 10 Nov 2022 16:31:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11478: Cleanup JniCatalog
Peter Rozsa has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19198 Change subject: IMPALA-11478: Cleanup JniCatalog .. IMPALA-11478: Cleanup JniCatalog This change adds a new class called JniCatalogOp which extracts the time calculation, error handling, thread annotation and logging from the operations in JniCatalog. Frontend test added as JniCatalogOpTest. Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab --- M fe/src/main/java/org/apache/impala/service/JniCatalog.java A fe/src/main/java/org/apache/impala/service/JniCatalogOp.java A fe/src/test/java/org/apache/impala/service/JniCatalogOpTest.java 3 files changed, 298 insertions(+), 231 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/19198/2 -- To view, visit http://gerrit.cloudera.org:8080/19198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1932172e2d13a7aab2336661c18befb4407ec9ab Gerrit-Change-Number: 19198 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Rozsa Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Quanlong Huang
