You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/11/23 19:42:09 UTC
[2/4] impala git commit: 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>
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 <fw...@cloudera.com>
Authored: Tue Nov 13 20:15:25 2018 -0800
Committer: Impala Public Jenkins <im...@cloudera.com>
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_; }