[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Feb 2019 02:35:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." Tests: Verified the patch manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Reviewed-on: http://gerrit.cloudera.org:8080/11228 Reviewed-by: Bharath Vissapragada Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 292 insertions(+), 43 deletions(-) Approvals: Bharath Vissapragada: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 8 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3809/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 22:37:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 22:37:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2180/ : 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/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 21:09:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 6: Fixed typo and rebase on master (again). -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 20:37:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has uploaded a new patch set (#7) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." Tests: Verified the patch manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 292 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/7 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2178/ : 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/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 19:55:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2175/ : 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/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 19:42:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has uploaded a new patch set (#6) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." Tests: Verified the patch manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 292 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/6 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2174/ : 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/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 19:32:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11228/5/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/11228/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@356 PS5, Line 356: Attemting > nit: typo Done -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 19:13:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11228/5/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/11228/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@356 PS5, Line 356: Attemting nit: typo -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 19:00:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has uploaded a new patch set (#5) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." Tests: Verified the patch manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 292 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/5 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11228/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/11228/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2436
PS4, Line 2436: "Get Partial Catalog Object - " +
Catalog.toCatalogObjectKey(req.object_desc))) {
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c
Gerrit-Change-Number: 11228
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 20 Feb 2019 18:52:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/11228/3/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/11228/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@356 PS3, Line 356: Attemting to > nit: I think this should be more like, "Attempting to lock table: "... Trying to keep name terse, but reworded as requested. http://gerrit.cloudera.org:8080/#/c/11228/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2436 PS3, Line 2436: + Catalog.toCatalogObjectKey(req.object_ > I think you should do Catalog.toCatalogObjectKey(req.object_desc); Done -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 18:51:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." Tests: Verified the patch manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 291 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/4 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 3: Code-Review+2 (2 comments) Couple of nits, other lgtm. http://gerrit.cloudera.org:8080/#/c/11228/3/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/11228/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@356 PS3, Line 356: Locking table nit: I think this should be more like, "Attempting to lock table: "... http://gerrit.cloudera.org:8080/#/c/11228/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2436 PS3, Line 2436: req.getObject_desc().getType().toString()) I think you should do Catalog.toCatalogObjectKey(req.object_desc); -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 18:09:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2167/ : 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/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 02:29:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." Tests: Verified the patch manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 291 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/3 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations
..
Patch Set 2:
(6 comments)
Addressed review comments and rebased.
http://gerrit.cloudera.org:8080/#/c/11228/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11228/2//COMMIT_MSG@24
PS2, Line 24: This patch is tricky to automate tests for, but I verified it
manually
: by jstacking a catalogd while performing some workload. Also
added a
: simple unit test to verify the thread renaming behavior
> Can be removed I guess.
Done
http://gerrit.cloudera.org:8080/#/c/11228/2/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/11228/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@301
PS2, Line 301: long end;
> Annotate here? This is one of those common interesting entry points for tab
Done
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2185
PS2, Line 2185: private TGetPartialCatalogObjectResponse
doGetPartialCatalogObject(
> This is another interesting entry point RPC for catalog v2 stuff. Add some
Done
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java
File fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java:
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java@24
PS2, Line 24: public class ThreadNameAnnotatorTest {
> great test :-)
Thanks. Handy little item from the bag-o-tricks...
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java@44
PS2, Line 44: wait();
> nit: I hope nothing hangs here forever due to a faulty test? Add a largish
Done
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java@107
PS2, Line 107: public void testExternalRename() throws InterruptedException {
> nit: Add a doc of what it does? Probably difficult to understand without an
Done
--
To view, visit http://gerrit.cloudera.org:8080/11228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c
Gerrit-Change-Number: 11228
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 20 Feb 2019 02:12:17 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations
..
Patch Set 2:
(6 comments)
Lgtm. There are quite a few places where we could use this annotator but that
is definitely out of scope for this patch. There are a couple of interesting
ones I pointed to in the comments that we could start with. Really liked the
test, thanks for adding it :-)
http://gerrit.cloudera.org:8080/#/c/11228/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11228/2//COMMIT_MSG@24
PS2, Line 24: This patch is tricky to automate tests for, but I verified it
manually
: by jstacking a catalogd while performing some workload. Also
added a
: simple unit test to verify the thread renaming behavior
Can be removed I guess.
http://gerrit.cloudera.org:8080/#/c/11228/2/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/11228/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@301
PS2, Line 301: long end;
Annotate here? This is one of those common interesting entry points for table
locks where the threads are usually hung.
Waiting to lock .
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2185
PS2, Line 2185: private TGetPartialCatalogObjectResponse
doGetPartialCatalogObject(
This is another interesting entry point RPC for catalog v2 stuff. Add some
annotations here?
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java
File fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java:
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java@24
PS2, Line 24: public class ThreadNameAnnotatorTest {
great test :-)
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java@44
PS2, Line 44: wait();
nit: I hope nothing hangs here forever due to a faulty test? Add a largish
timeout?
http://gerrit.cloudera.org:8080/#/c/11228/2/fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java@107
PS2, Line 107: public void testExternalRename() throws InterruptedException {
nit: Add a doc of what it does? Probably difficult to understand without any
context.
--
To view, visit http://gerrit.cloudera.org:8080/11228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c
Gerrit-Change-Number: 11228
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Sun, 27 Jan 2019 06:52:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1899/ : 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/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 26 Jan 2019 02:38:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 2: (5 comments) Applied code review comments. Added a test case. Bharath, can you give this a review and see if it is ready to go? http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG@25 PS1, Line 25: by jstacking a catalogd while performing some workload. Also added a > k, I'll add a simple test. I think just doing it single-threaded and assert Created an automated, synchronized test. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@105 PS1, Line 105: sw.elapsedTime(TimeUnit.MILLISECONDS) > Can this be rolled into ThreadNameAnnotator (or InstrumentedScope) and log Could, but requires passing the logger into the annotator so the the log message is attributed to the proper class. Also, would have to pass in the log text. Actually, seems simpler to do it as shown here: the worker thread has much more context than the annotator does. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java File fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@26 PS1, Line 26: * reading a jstack. For example, when making calls to external services which might > It's obvious, but perhaps mention that this isn't thread-safe. Added a bit more explanation. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51 PS1, Line 51: thr_.setName(newName_); > nit: I think this could fit on one line? Done http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51 PS1, Line 51: .setName(newName_); > When is this possible? (Thread.currentThread() != thr_) Same question. Since the annotator sits in the executing thread itself (not outside), I can't think of a way that the thread can replace itself. In fact, the only way this could happen is if someone made the mistake of setting the thread name in one thread, and calling close in another. Changed to use Preconditions to check for this error case. -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 26 Jan 2019 02:10:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Paul Rogers has uploaded a new patch set (#2) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." This patch is tricky to automate tests for, but I verified it manually by jstacking a catalogd while performing some workload. Also added a simple unit test to verify the thread renaming behavior. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java A fe/src/test/java/org/apache/impala/util/ThreadNameAnnotatorTest.java 5 files changed, 245 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/2 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 1: (2 comments) Great supportability add. I think we should get this in asap. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@105 PS1, Line 105: sw.elapsedTime(TimeUnit.MILLISECONDS) Can this be rolled into ThreadNameAnnotator (or InstrumentedScope) and log it in Close()? Also Prettyprint it? Eventually we can use this for all DDLs/Catalog OPs http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java File fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51 PS1, Line 51: Thread.currentThread() == thr_ When is this possible? (Thread.currentThread() != thr_) -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 16 Aug 2018 21:56:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java
File fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java:
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@48
PS1, Line 48: public void close() {
> maybe I'll evolve this class into something a bit more broad like:
This seems nice. Can be done later, obviously.
--
To view, visit http://gerrit.cloudera.org:8080/11228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c
Gerrit-Change-Number: 11228
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 15 Aug 2018 23:51:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG@25
PS1, Line 25: This patch is tricky to automate tests for, but I verified it
manually
> What do you think of the following test:
k, I'll add a simple test. I think just doing it single-threaded and asserting
on the expected thread name from the same thread is probably easiest instead of
futzing with semaphores.
http://gerrit.cloudera.org:8080/#/c/11228/1/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/11228/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1005
PS1, Line 1005: String annotation = String.format("invalidating
metadata - %s/%s dbs complete",
> In an ideal world, the RPC from the impalad would be carrying a thread id,
yea, distributed tracing is also on my list, but wanted to start with this
simple incremental improvement before I open the can of worms of which
framework to use (eg opentracing vs opencensus)
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java
File fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java:
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@48
PS1, Line 48: public void close() {
> Do we want this to also double as something that logs long requests? i.e.,
maybe I'll evolve this class into something a bit more broad like:
try (InstrumentedScope s = new InstrumentedScope("blah blah")
.annotateThreadName()
.logIfSlow(1, TimeUnit.SECONDS)
.incrementMetric(METRIC_LOAD_TIME)
.addToProfile("FooTime"))
etc. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/11228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c
Gerrit-Change-Number: 11228
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 15 Aug 2018 23:46:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations
..
Patch Set 1:
(5 comments)
This seems lovely to me and a good idea. I'm happy with it as is, except that
it seems like a simple test of the annotator is plausible.
I think we log thread names when we log from Java. Does that look reasonable to
you?
http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG@25
PS1, Line 25: This patch is tricky to automate tests for, but I verified it
manually
What do you think of the following test:
countdownLatchThingyInitializedToOne
annotatedThread = {
try(annotater) { grap semaphore; }
}.run()
checkusingJmxJstack that thread is annotated
release the semaphore.
To make it work you might need a second semaphore to lockstep the threads, but
you get the idea.
I'm open to this being too annoying, but it seems quick enough.
http://gerrit.cloudera.org:8080/#/c/11228/1/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/11228/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1005
PS1, Line 1005: String annotation = String.format("invalidating
metadata - %s/%s dbs complete",
In an ideal world, the RPC from the impalad would be carrying a thread id, and
this would have a thread id in it as well. Obviously not an actionable comment
right now, but since you're knee-deep in that path, I wanted to mention it.
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java
File fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java:
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@26
PS1, Line 26: * that is blocked or which service is being waited upon.
It's obvious, but perhaps mention that this isn't thread-safe.
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@48
PS1, Line 48: public void close() {
Do we want this to also double as something that logs long requests? i.e.,
should we log if things take more than 1 second?
http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51
PS1, Line 51: if (Thread.currentThread() == thr_ &&
nit: I think this could fit on one line?
--
To view, visit http://gerrit.cloudera.org:8080/11228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c
Gerrit-Change-Number: 11228
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Wed, 15 Aug 2018 16:19:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 ) Change subject: IMPALA-7450. Set thread name during refresh/load operations .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/340/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 15 Aug 2018 06:55:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7450. Set thread name during refresh/load operations
Hello Bharath Vissapragada, Philip Zeyliger, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11228 to review the following change. Change subject: IMPALA-7450. Set thread name during refresh/load operations .. IMPALA-7450. Set thread name during refresh/load operations This adds a small utility class for annotating the current thread's name during potentially long-running operations such as refresh/load. With this change, jstack output now includes useful thread names like: During startup: "main [invalidating metadata - 128/428 dbs complete]" While loading a fresh table: "pool-4-thread-12 [Loading metadata for: foo_db.foo_table] [Loading metadata for all partition(s) of foo_db.foo_table]" Pool refreshing metadata for a particular path: "pool-23-thread-5 [Refreshing file metadata for path: hdfs://nameservice1/path/to/partdir..." This patch is tricky to automate tests for, but I verified it manually by jstacking a catalogd while performing some workload. Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/TableLoader.java A fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java 4 files changed, 99 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11228/1 -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Philip Zeyliger
