[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 25 Jul 2018 23:29:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/60/ : 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/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 25 Jul 2018 20:56:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, Vuk Ercegovac, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11026 to look at the new patch set (#3). Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. IMPALA-7340. Only send necessary fields in THdfsPartition The THdfsPartition Thrift struct is used both for partition metadata (sent from catalogd to coordinator) and for descriptors (sent from coordinator frontend to backends). In the case of the descriptor, not all fields are actually used. This patch cleans up the Thrift struct definition to be more clear which fields are used where, and changes the serialization code to only fill in the necessary fields. Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 --- M common/thrift/CatalogObjects.thrift M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java 5 files changed, 90 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/11026/3 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 25 Jul 2018 05:38:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/36/ : 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/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 24 Jul 2018 19:48:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 2: Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/36/ Running initial code review checks. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 24 Jul 2018 18:46:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Hello Bharath Vissapragada, Tianyi Wang, Vuk Ercegovac, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11026 to look at the new patch set (#2). Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. IMPALA-7340. Only send necessary fields in THdfsPartition The THdfsPartition Thrift struct is used both for partition metadata (sent from catalogd to coordinator) and for descriptors (sent from coordinator frontend to backends). In the case of the descriptor, not all fields are actually used. This patch cleans up the Thrift struct definition to be more clear which fields are used where, and changes the serialization code to only fill in the necessary fields. Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 --- M common/thrift/CatalogObjects.thrift M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java 5 files changed, 90 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/11026/2 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift@263 PS1, Line 263: If -1, the partition does not currently : // exist. > I think this should probably refer to PROTOTYPE_PARTITION_ID. Will change Done http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift@327 PS1, Line 327: // Each TNetworkAddress is a datanode which contains blocks of a file in the table. > Will do Done http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1722 PS1, Line 1722:* included. Otherwise, don't include any THdfsFileDescs, and include only those partitions > Yea, we usually stick to 90 chars. Done -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 24 Jul 2018 18:27:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1722 PS1, Line 1722:* included. Otherwise, don't include any THdfsFileDescs, and include only those partitions > What's the guidance here? The Google guide says 100 chars for Java: https:/ Yea, we usually stick to 90 chars. -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 24 Jul 2018 16:38:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift@263 PS1, Line 263: If -1, the partition does not currently : // exist. > Skimmed through the code, I don't see where we set it to -1. I think this should probably refer to PROTOTYPE_PARTITION_ID. Will change http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift@327 PS1, Line 327: // Each TNetworkAddress is a datanode which contains blocks of a file in the table. > Should we add a similar demarcation here? FULL vs DESCRIPTOR... Will do http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1722 PS1, Line 1722:* included. Otherwise, don't include any THdfsFileDescs, and include only those partitions > nit: longline What's the guidance here? The Google guide says 100 chars for Java: https://google.github.io/styleguide/javaguide.html#s4.4-column-limit https://cwiki.apache.org/confluence/display/IMPALA/Impala+Style+Guide says 90 I guess looking at the actual distribution in the Java code, there seems to be a drop-off around 90: 79: 1155 80: 1160 81: 1287 82: 1273 83: 1200 84: 1274 85: 1300 86: 1235 87: 1275 88: 1157 89: 982 90: 434 91: 52 92: 41 93: 33 94: 18 95: 19 96: 11 97: 7 98: 10 99: 9 100: 12 101: 9 102: 8 103: 9 104: 9 105: 8 106: 12 107: 6 108: 12 109: 14 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 24 Jul 2018 16:06:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift@263 PS1, Line 263: If -1, the partition does not currently : // exist. Skimmed through the code, I don't see where we set it to -1. http://gerrit.cloudera.org:8080/#/c/11026/1/common/thrift/CatalogObjects.thrift@327 PS1, Line 327: // Each TNetworkAddress is a datanode which contains blocks of a file in the table. Should we add a similar demarcation here? FULL vs DESCRIPTOR... http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/11026/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1722 PS1, Line 1722:* included. Otherwise, don't include any THdfsFileDescs, and include only those partitions nit: longline -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 24 Jul 2018 07:25:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11026 ) Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. Patch Set 1: Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/24/ Running initial code review checks. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Mon, 23 Jul 2018 23:58:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition
Hello Bharath Vissapragada, Tianyi Wang, Vuk Ercegovac, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11026 to review the following change. Change subject: IMPALA-7340. Only send necessary fields in THdfsPartition .. IMPALA-7340. Only send necessary fields in THdfsPartition The THdfsPartition Thrift struct is used both for partition metadata (sent from catalogd to coordinator) and for descriptors (sent from coordinator frontend to backends). In the case of the descriptor, not all fields are actually used. This patch cleans up the Thrift struct definition to be more clear which fields are used where, and changes the serialization code to only fill in the necessary fields. Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 --- M common/thrift/CatalogObjects.thrift M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java 5 files changed, 71 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/11026/1 -- To view, visit http://gerrit.cloudera.org:8080/11026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0 Gerrit-Change-Number: 11026 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac
