You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/07/23 23:57:54 UTC

[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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 tarmstrong@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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 18:46:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/24/ : 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: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 00:49:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 Started https://jenkins.impala.io/job/gerrit-code-review-checks/60/ 

Running initial code review checks. This is experimental - please report any issues to tarmstrong@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: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 20:07:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 16:38:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 tarmstrong@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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 23:58:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2864/ DRY_RUN=true


-- 
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 20:08:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 3: 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: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jul 2018 00:14:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 23:29:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 18:27:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 05:38:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 19:48:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jul 2018 20:56:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 16:06:03 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11026 )

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
Reviewed-on: http://gerrit.cloudera.org:8080/11026
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
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(-)

Approvals:
  Impala Public Jenkins: Verified
  Todd Lipcon: Looks good to me, approved

-- 
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: merged
Gerrit-Change-Id: I97e8402efdfdeea06463bb71a40ebb6abd1f11f0
Gerrit-Change-Number: 11026
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7340. Only send necessary fields in THdfsPartition

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 07:25:21 +0000
Gerrit-HasComments: Yes