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 <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/8c59f0dc Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/8c59f0dc Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/8c59f0dc Branch: refs/heads/master Commit: 8c59f0dcef11389ebfde1788aa3237b7b31ab3a4 Parents: d4019be Author: Fredy Wijaya <[email protected]> Authored: Tue Nov 13 20:15:25 2018 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Nov 22 01:37:30 2018 +0000 ---------------------------------------------------------------------- .../java/org/apache/impala/catalog/Catalog.java | 8 +++++++- .../impala/catalog/CatalogObjectImpl.java | 20 +++++++++++++++++--- .../impala/catalog/CatalogServiceCatalog.java | 5 +++-- .../org/apache/impala/catalog/DataSource.java | 10 +++------- .../main/java/org/apache/impala/catalog/Db.java | 10 +++------- .../org/apache/impala/catalog/Function.java | 13 +++---------- .../apache/impala/catalog/HdfsCachePool.java | 6 +++++- .../org/apache/impala/catalog/Principal.java | 11 +---------- .../impala/catalog/PrincipalPrivilege.java | 11 ++--------- .../java/org/apache/impala/catalog/Table.java | 15 +++++---------- 10 files changed, 49 insertions(+), 60 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/Catalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Catalog.java b/fe/src/main/java/org/apache/impala/catalog/Catalog.java index 4a14428..2a8f5a0 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Catalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/Catalog.java @@ -544,9 +544,15 @@ public abstract class Catalog { /** * Returns a unique string key of a catalog object. + * + * This method may initially seem counter-intuitive because Catalog::getUniqueName() + * uses this method to build a unique name instead of Catalog::getUniqueName() + * providing the implementation on how to build a catalog object key. The reason is + * building CatalogObject from TCatalogObject in order to call getUniqueName() can + * be an expensive operation, especially for constructing a Table catalog object + * from TCatalogObject. */ public static String toCatalogObjectKey(TCatalogObject catalogObject) { - // TODO (IMPALA-7839): Refactor this method to reduce code repetition. Preconditions.checkNotNull(catalogObject); switch (catalogObject.getType()) { case DATABASE: http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java b/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java index 321355c..20351df 100644 --- a/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java +++ b/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java @@ -19,8 +19,7 @@ package org.apache.impala.catalog; import java.util.concurrent.atomic.AtomicLong; -import org.apache.impala.common.NotImplementedException; -import org.apache.impala.thrift.TCatalogObjectType; +import org.apache.impala.thrift.TCatalogObject; abstract public class CatalogObjectImpl implements CatalogObject { // Current catalog version of this object. Initialized to @@ -42,6 +41,21 @@ abstract public class CatalogObjectImpl implements CatalogObject { public String getName() { return ""; } @Override - public String getUniqueName() { return ""; } + public final String getUniqueName() { + return Catalog.toCatalogObjectKey(toTCatalogObject()); + } + + public final TCatalogObject toTCatalogObject() { + TCatalogObject catalogObject = + new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); + setTCatalogObject(catalogObject); + return catalogObject; + } + + /** + * A template method used by {@link CatalogObjectImpl#toTCatalogObject()} for + * populating the given catalogObject fields. + */ + protected abstract void setTCatalogObject(TCatalogObject catalogObject); } http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java index 9533507..b2f113b 100644 --- a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java @@ -808,13 +808,14 @@ public class CatalogServiceCatalog extends Catalog { try { long tblVersion = tbl.getCatalogVersion(); if (tblVersion <= ctx.fromVersion) return; + String tableUniqueName = tbl.getUniqueName(); TopicUpdateLog.Entry topicUpdateEntry = - topicUpdateLog_.getOrCreateLogEntry(tbl.getUniqueName()); + topicUpdateLog_.getOrCreateLogEntry(tableUniqueName); if (tblVersion > ctx.toVersion && topicUpdateEntry.getNumSkippedTopicUpdates() < MAX_NUM_SKIPPED_TOPIC_UPDATES) { LOG.info("Table " + tbl.getFullName() + " is skipping topic update " + ctx.toVersion); - topicUpdateLog_.add(tbl.getUniqueName(), + topicUpdateLog_.add(tableUniqueName, new TopicUpdateLog.Entry( topicUpdateEntry.getNumSkippedTopicUpdates() + 1, topicUpdateEntry.getLastSentVersion(), http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/DataSource.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/DataSource.java b/fe/src/main/java/org/apache/impala/catalog/DataSource.java index 527b187..043902d 100644 --- a/fe/src/main/java/org/apache/impala/catalog/DataSource.java +++ b/fe/src/main/java/org/apache/impala/catalog/DataSource.java @@ -53,8 +53,6 @@ public class DataSource extends CatalogObjectImpl implements FeDataSource { @Override public String getName() { return dataSrcName_; } - @Override - public String getUniqueName() { return "DATA_SOURCE:" + dataSrcName_.toLowerCase(); } @Override // FeDataSource public String getLocation() { return location_; } @Override // FeDataSource @@ -79,10 +77,8 @@ public class DataSource extends CatalogObjectImpl implements FeDataSource { return fromThrift(thrift).debugString(); } - public TCatalogObject toTCatalogObject() { - TCatalogObject catalogObj = - new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); - catalogObj.setData_source(toThrift()); - return catalogObj; + @Override + protected void setTCatalogObject(TCatalogObject catalogObject) { + catalogObject.setData_source(toThrift()); } } http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/Db.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Db.java b/fe/src/main/java/org/apache/impala/catalog/Db.java index f5bc0b2..6bad98c 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Db.java +++ b/fe/src/main/java/org/apache/impala/catalog/Db.java @@ -138,8 +138,6 @@ public class Db extends CatalogObjectImpl implements FeDb { public String getName() { return thriftDb_.getDb_name(); } @Override public TCatalogObjectType getCatalogObjectType() { return TCatalogObjectType.DATABASE; } - @Override - public String getUniqueName() { return "DATABASE:" + getName().toLowerCase(); } /** * Adds a table to the table cache. @@ -434,11 +432,9 @@ public class Db extends CatalogObjectImpl implements FeDb { } } - public TCatalogObject toTCatalogObject() { - TCatalogObject catalogObj = - new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); - catalogObj.setDb(toThrift()); - return catalogObj; + @Override + protected void setTCatalogObject(TCatalogObject catalogObject) { + catalogObject.setDb(toThrift()); } /** http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/Function.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Function.java b/fe/src/main/java/org/apache/impala/catalog/Function.java index e0c98be..9b2883b 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Function.java +++ b/fe/src/main/java/org/apache/impala/catalog/Function.java @@ -302,20 +302,13 @@ public class Function extends CatalogObjectImpl { public TCatalogObjectType getCatalogObjectType() { return TCatalogObjectType.FUNCTION; } @Override public String getName() { return getFunctionName().toString(); } - @Override - public String getUniqueName() { - return "FUNCTION:" + name_.toString() + "(" + signatureString() + ")"; - } // Child classes must override this function. public String toSql(boolean ifNotExists) { return ""; } - public TCatalogObject toTCatalogObject () { - TCatalogObject result = new TCatalogObject(); - result.setType(TCatalogObjectType.FUNCTION); - result.setFn(toThrift()); - result.setCatalog_version(getCatalogVersion()); - return result; + @Override + protected void setTCatalogObject(TCatalogObject catalogObject) { + catalogObject.setFn(toThrift()); } public TFunction toThrift() { http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java b/fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java index 6f752d4..6e3bedc 100644 --- a/fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java +++ b/fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java @@ -19,6 +19,7 @@ package org.apache.impala.catalog; import org.apache.hadoop.hdfs.protocol.CachePoolInfo; +import org.apache.impala.thrift.TCatalogObject; import org.apache.impala.thrift.TCatalogObjectType; import org.apache.impala.thrift.THdfsCachePool; import com.google.common.base.Preconditions; @@ -55,6 +56,9 @@ public class HdfsCachePool extends CatalogObjectImpl { @Override public String getName() { return cachePool_.getPool_name(); } + @Override - public String getUniqueName() { return "HDFS_CACHE_POOL:" + getName().toLowerCase(); } + protected void setTCatalogObject(TCatalogObject catalogObject) { + catalogObject.setCache_pool(toThrift()); + } } http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/Principal.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Principal.java b/fe/src/main/java/org/apache/impala/catalog/Principal.java index 8f65e95..650eac4 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Principal.java +++ b/fe/src/main/java/org/apache/impala/catalog/Principal.java @@ -158,17 +158,8 @@ public abstract class Principal extends CatalogObjectImpl { public int getId() { return principal_.getPrincipal_id(); } @Override - public String getUniqueName() { - String principalName = getPrincipalType() == TPrincipalType.ROLE ? - getName().toLowerCase() : getName(); - return "PRINCIPAL:" + principalName + "." + getPrincipalType().name(); - } - - public TCatalogObject toTCatalogObject() { - TCatalogObject catalogObject = - new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); + protected void setTCatalogObject(TCatalogObject catalogObject) { catalogObject.setPrincipal(toThrift()); - return catalogObject; } /** http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java index c375cfc..4876367 100644 --- a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java +++ b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java @@ -143,17 +143,10 @@ public class PrincipalPrivilege extends CatalogObjectImpl { public String getName() { return buildPrivilegeName(privilege_); } public int getPrincipalId() { return privilege_.getPrincipal_id(); } public TPrincipalType getPrincipalType() { return privilege_.getPrincipal_type(); } - @Override - public String getUniqueName() { - return "PRIVILEGE:" + getName().toLowerCase() + "." + Integer.toString( - getPrincipalId()) + "." + getPrincipalType().toString(); - } - public TCatalogObject toTCatalogObject() { - TCatalogObject catalogObject = - new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); + @Override + protected void setTCatalogObject(TCatalogObject catalogObject) { catalogObject.setPrivilege(toThrift()); - return catalogObject; } // The time this principal was created. Used to quickly check if the same privilege http://git-wip-us.apache.org/repos/asf/impala/blob/8c59f0dc/fe/src/main/java/org/apache/impala/catalog/Table.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Table.java b/fe/src/main/java/org/apache/impala/catalog/Table.java index 5308ca6..97cbd62 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Table.java +++ b/fe/src/main/java/org/apache/impala/catalog/Table.java @@ -373,13 +373,6 @@ public abstract class Table extends CatalogObjectImpl implements FeTable { return table; } - public TCatalogObject toTCatalogObject() { - TCatalogObject catalogObject = - new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); - catalogObject.setTable(toThrift()); - return catalogObject; - } - public TCatalogObject toMinimalTCatalogObject() { TCatalogObject catalogObject = new TCatalogObject(getCatalogObjectType(), getCatalogVersion()); @@ -389,6 +382,11 @@ public abstract class Table extends CatalogObjectImpl implements FeTable { return catalogObject; } + @Override + protected void setTCatalogObject(TCatalogObject catalogObject) { + catalogObject.setTable(toThrift()); + } + /** * Return partial info about this table. This is called only on the catalogd to * service GetPartialCatalogObject RPCs. @@ -454,9 +452,6 @@ public abstract class Table extends CatalogObjectImpl implements FeTable { return new TableName(db_ != null ? db_.getName() : null, name_); } - @Override // CatalogObject - public String getUniqueName() { return "TABLE:" + getFullName(); } - @Override // FeTable public ArrayList<Column> getColumns() { return colsByPos_; }
