You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/11/14 04:22:08 UTC

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name 

Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11928


Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................

IMPALA-7839: Remove code duplication for getting a unique catalog object name

This patch removes code duplication for getting a catalog object
unique name by reusing the code from Catalog::toCatalogObjectKey(
TCatalogObject).

Testing:
- Ran all FE tests

Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
9 files changed, 17 insertions(+), 27 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11928/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45:     return Catalog.toCatalogObjectKey(toTCatalogObject());
> This is a fair point. Initially I thought of doing that, too. Given the cur
Doesn't something like the following work? Just an example for DataSource but other cases are similar too. Do we need to do something extra?

 public static String toCatalogObjectKey(TCatalogObject catalogObject) {
    Preconditions.checkNotNull(catalogObject);
    switch (catalogObject.getType()) {
      .......
    case DATA_SOURCE:
        return DataSource.fromThrift(catalogObject.getData_source()).getUniqueName();
       ...........
     }


http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58
PS2, Line 58:   protected abstract void writeToCatalogObject(TCatalogObject catalogObject);
> I know this is the convention that we in some places, however I'm kind of t
Ya, the compile time enforcement is missing unless we separate it out. How about renaming it to setTCatalogObject()? to be consistent with thrift's set*()? 

Also could you add a little more detail in the method doc. Mention that it is used to toTCatalog..()?



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 17:49:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45:     return Catalog.toCatalogObjectKey(toTCatalogObject());
> The problem is with Table: https://github.com/apache/impala/blob/master/fe/
good point. Table.fromThrift() is probably expensive. Mind adding a method doc here with some background and why we do it this way? Doesn't look quite intuitive for first-time readers.



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 20:50:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 1: Code-Review+1

(2 comments)

lgtm + I left some ideas.

http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@44
PS1, Line 44:   public String getUniqueName() { return Catalog.toCatalogObjectKey(toTCatalogObject()); }
Does any subclass overrides this? If not, then it could be 'final'.


http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:

http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java@439
PS1, Line 439:     catalogObj.setDb(toThrift());
+1 idea to remove code duplication:
as toTCatalogObject() implementations seem identical with the exception of setting different properties, the catalogObj.set* part could be moved to a function like writeToTCatalogObject, while the rest of toTCatalogObject() could be moved to CatalogObjectImpl.



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 15:50:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:33:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 4: Code-Review+2

(1 comment)

Carry Bharath's +2.

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45:     return Catalog.toCatalogObjectKey(toTCatalogObject());
> good point. Table.fromThrift() is probably expensive. Mind adding a method 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:32:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1363/ : 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/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Nov 2018 04:56:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45:     return Catalog.toCatalogObjectKey(toTCatalogObject());
> Converting an object to it's thrift struct to get it's unique name seems ki
This is a fair point. Initially I thought of doing that, too. Given the current code, I can't think of an elegant way of using CatalogObject in Catalog. Let me know if you have some suggestion.


http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58
PS2, Line 58:   protected abstract void writeToCatalogObject(TCatalogObject catalogObject);
> Rather than having 2 methods here, how about just overriding toTCatalogObje
I know this is the convention that we in some places, however I'm kind of torn with using this style especially since there is mo safe way of enforcing that the subclass needs to call super.toTCatalogObject(). I've seen many places in Impala that uses this style and some subclasses call super and some subclasses just duplicate the code that's already been implemented in the super class. Another issue that I have with this style is there's no compile-time check that says the subclass must override toTCatalogObject(). If there's a new CatalogObject that simply extends CatalogObjectImpl without overriding toTCatalogObject(), the code will still compile fine but it will be broken at runtime.



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 02:32:03 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................

IMPALA-7839: Remove code duplication for getting a unique catalog object name

This patch removes code duplication for getting a catalog object
unique name by reusing the code from Catalog::toCatalogObjectKey(
TCatalogObject).

Testing:
- Ran all FE tests

Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
10 files changed, 49 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11928/4
-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45:     return Catalog.toCatalogObjectKey(toTCatalogObject());
> Doesn't something like the following work? Just an example for DataSource b
The problem is with Table: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/Table.java#L273-L289. Basically we need the Db object which is not available. Calling catalogObject.getDb() will be null when catalogObject.getType() is TABLE or VIEW. Furthermore, the Table::fromThrift() seems to do a lot more stuff, which can be somewhat expensive if the goal is to just get a unique name.

IMHO the unnecessary translation from TCatalogObject to CatalogObject in order to get unique name isn't very efficient especially since toCatalogObjectKey is called a lot of times whereas getUniqueName() is only called in one place and only for Table catalog object: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java#L781-L794. Unfortunately since getUniqueName() is a method required by Catalog interface, all Catalog implementations need to implement it, but they're not actually used other than by Table catalog object.


http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58
PS2, Line 58:    */
> Ya, the compile time enforcement is missing unless we separate it out. How 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 18:56:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 2:

(2 comments)

Have a couple of questions, want to know your thoughts on them.

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45
PS2, Line 45:     return Catalog.toCatalogObjectKey(toTCatalogObject());
Converting an object to it's thrift struct to get it's unique name seems kinda roundabout to me. What do you think? 

Also if someone wants to change the unique key for a given class, they need go through this call chain and change toCatalogObjectKey() in the Catalog class which is weird.

Ideally I'd expect each CatalogObject to be able to define it's own unique name impl (override getUniqueName()) and Catalog#toCatalogObjectKey() to rely on the overriden methods.


http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58
PS2, Line 58:   protected abstract void writeToCatalogObject(TCatalogObject catalogObject);
Rather than having 2 methods here, how about just overriding toTCatalogObject() in the base class?

For example for a data source you'd do something like

public final TCatalogObject toTCatalogObject() {
  return super.toTCatalogObject().setData_source(toThrift());
}

Don't think there is a way to enforce the overrding part but that appears more cleaner (we follow that approach for toThrift() overrides)



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 22:48:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1421/ : 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/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 19:33:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java:

http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@44
PS1, Line 44:   public final String getUniqueName() {
> Does any subclass overrides this? If not, then it could be 'final'.
Yeah final is a good idea. We can remove it later if that's not the case. Done.


http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:

http://gerrit.cloudera.org:8080/#/c/11928/1/fe/src/main/java/org/apache/impala/catalog/Db.java@439
PS1, Line 439: 
> +1 idea to remove code duplication:
Good suggestion. Done.



-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 16:21:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1406/ : 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/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 17:03:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3487/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:33:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 5: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Nov 2018 01:37:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 2: Code-Review+1

Carry Csaba's +1.


-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 16:21:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................

IMPALA-7839: Remove code duplication for getting a unique catalog object name

This patch removes code duplication for getting a catalog object
unique name by reusing the code from Catalog::toCatalogObjectKey(
TCatalogObject).

Testing:
- Ran all FE tests

Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Reviewed-on: http://gerrit.cloudera.org:8080/11928
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
10 files changed, 49 insertions(+), 60 deletions(-)

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

-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................

IMPALA-7839: Remove code duplication for getting a unique catalog object name

This patch removes code duplication for getting a catalog object
unique name by reusing the code from Catalog::toCatalogObjectKey(
TCatalogObject).

Testing:
- Ran all FE tests

Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
9 files changed, 38 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11928/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................

IMPALA-7839: Remove code duplication for getting a unique catalog object name

This patch removes code duplication for getting a catalog object
unique name by reusing the code from Catalog::toCatalogObjectKey(
TCatalogObject).

Testing:
- Ran all FE tests

Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
10 files changed, 42 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/11928/3
-- 
To view, visit http://gerrit.cloudera.org:8080/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7839: Remove code duplication for getting a unique catalog object name

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )

Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1425/ : 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/11928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f
Gerrit-Change-Number: 11928
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 22:12:12 +0000
Gerrit-HasComments: No