This is an automated email from the ASF dual-hosted git repository. prozsa pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit ad19828df5ab45704c9b034813dd67a1571d3f45 Author: Peter Rozsa <[email protected]> AuthorDate: Thu Apr 17 13:17:54 2025 +0200 IMPALA-14040: Remove Kudu masters property from FeCatalog This change removes getDefaultKuduMasterHosts from FeCatalog and makes the Kudu masters lookup bound directly to the backend config. Tests are adjusted for the new property lookup. Change-Id: Idcf31a724bb7bd00268accf21c0b997d92e9b23a Reviewed-on: http://gerrit.cloudera.org:8080/22863 Reviewed-by: Impala Public Jenkins <[email protected]> Reviewed-by: Daniel Becker <[email protected]> Tested-by: Daniel Becker <[email protected]> --- .../java/org/apache/impala/analysis/CreateTableStmt.java | 16 ++++++++-------- .../main/java/org/apache/impala/catalog/FeCatalog.java | 2 -- .../java/org/apache/impala/catalog/ImpaladCatalog.java | 9 +-------- .../org/apache/impala/catalog/local/LocalCatalog.java | 9 +-------- .../java/org/apache/impala/service/FeCatalogManager.java | 8 +++----- .../java/org/apache/impala/analysis/AnalyzeDDLTest.java | 2 ++ .../org/apache/impala/analysis/AnalyzeKuduDDLTest.java | 12 +++++++++--- .../apache/impala/analysis/StmtMetadataLoaderTest.java | 2 +- .../test/java/org/apache/impala/analysis/ToSqlTest.java | 7 +++++-- .../impala/authorization/AuthorizationStmtTest.java | 1 + .../apache/impala/catalog/local/LocalCatalogTest.java | 2 +- .../java/org/apache/impala/planner/PlannerTestBase.java | 3 +++ .../org/apache/impala/testutil/ImpaladTestCatalog.java | 4 ++-- 13 files changed, 37 insertions(+), 40 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java index 1e9d0f6ef..fbf569120 100644 --- a/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java @@ -351,7 +351,7 @@ public class CreateTableStmt extends StatementBase implements SingleTableStmt { analyzeKuduTableProperties(analyzer); if (isExternalWithNoPurge()) { // this is an external table - analyzeExternalKuduTableParams(analyzer); + analyzeExternalKuduTableParams(); } else { // this is either a managed table or an external table with external.table.purge // property set to true @@ -364,7 +364,7 @@ public class CreateTableStmt extends StatementBase implements SingleTableStmt { * Kudu tables. */ private void analyzeKuduTableProperties(Analyzer analyzer) throws AnalysisException { - String kuduMasters = getKuduMasters(analyzer); + String kuduMasters = getKuduMasters(); if (kuduMasters.isEmpty()) { throw new AnalysisException(String.format( "Table property '%s' is required when the impalad startup flag " + @@ -432,10 +432,10 @@ public class CreateTableStmt extends StatementBase implements SingleTableStmt { * Populates Kudu master addresses either from table property or * the -kudu_master_hosts flag. */ - private String getKuduMasters(Analyzer analyzer) { + private String getKuduMasters() { String kuduMasters = getTblProperties().get(KuduTable.KEY_MASTER_HOSTS); if (Strings.isNullOrEmpty(kuduMasters)) { - kuduMasters = analyzer.getCatalog().getDefaultKuduMasterHosts(); + kuduMasters = BackendConfig.INSTANCE.getBackendCfg().kudu_master_hosts; } return kuduMasters; } @@ -443,7 +443,7 @@ public class CreateTableStmt extends StatementBase implements SingleTableStmt { /** * Analyzes and checks parameters specified for external Kudu tables. */ - private void analyzeExternalKuduTableParams(Analyzer analyzer) + private void analyzeExternalKuduTableParams() throws AnalysisException { Preconditions.checkState(!Boolean .parseBoolean(getTblProperties().get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE))); @@ -476,7 +476,7 @@ public class CreateTableStmt extends StatementBase implements SingleTableStmt { throw new AnalysisException(String.format("Table property '%s' cannot be set to " + "true with an managed Kudu table.", KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE)); } - analyzeSynchronizedKuduTableName(analyzer); + analyzeSynchronizedKuduTableName(); // Check column types are valid Kudu types for (ColumnDef col: getColumnDefs()) { @@ -513,12 +513,12 @@ public class CreateTableStmt extends StatementBase implements SingleTableStmt { * it in TableDef.generatedKuduTableName_. Throws if the Kudu table * name was given manually via TBLPROPERTIES. */ - private void analyzeSynchronizedKuduTableName(Analyzer analyzer) + private void analyzeSynchronizedKuduTableName() throws AnalysisException { AnalysisUtils.throwIfNotNull(getTblProperties().get(KuduTable.KEY_TABLE_NAME), String.format("Not allowed to set '%s' manually for synchronized Kudu tables.", KuduTable.KEY_TABLE_NAME)); - String kuduMasters = getKuduMasters(analyzer); + String kuduMasters = getKuduMasters(); boolean isHMSIntegrationEnabled; try { // Check if Kudu's integration with the Hive Metastore is enabled. Validation diff --git a/fe/src/main/java/org/apache/impala/catalog/FeCatalog.java b/fe/src/main/java/org/apache/impala/catalog/FeCatalog.java index 617c9e549..0e9acc581 100644 --- a/fe/src/main/java/org/apache/impala/catalog/FeCatalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/FeCatalog.java @@ -109,8 +109,6 @@ public interface FeCatalog { TUniqueId getCatalogServiceId(); AuthorizationPolicy getAuthPolicy(); - String getDefaultKuduMasterHosts(); - /** * Returns true if the catalog is ready to accept requests (has diff --git a/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java b/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java index 896179ffe..af834430c 100644 --- a/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java @@ -105,17 +105,12 @@ public class ImpaladCatalog extends Catalog implements FeCatalog { // Object that is used to synchronize on and signal when a catalog update is received. private final Object catalogUpdateEventNotifier_ = new Object(); - // The addresses of the Kudu masters to use if no Kudu masters were explicitly provided. - // Used during table creation. - private final String defaultKuduMasterHosts_; private final AtomicReference<? extends AuthorizationChecker> authzChecker_; - public ImpaladCatalog(String defaultKuduMasterHosts, - AtomicReference<? extends AuthorizationChecker> authzChecker) { + public ImpaladCatalog(AtomicReference<? extends AuthorizationChecker> authzChecker) { super(); authzChecker_ = authzChecker; addDb(BuiltinsDb.getInstance()); - defaultKuduMasterHosts_ = defaultKuduMasterHosts; // Ensure the contents of the CatalogObjectVersionSet instance are cleared when a // new instance of ImpaladCatalog is created (see IMPALA-6486). CatalogObjectVersionSet.INSTANCE.clear(); @@ -677,8 +672,6 @@ public class ImpaladCatalog extends Catalog implements FeCatalog { } @Override // FeCatalog public AuthorizationPolicy getAuthPolicy() { return authPolicy_; } - @Override // FeCatalog - public String getDefaultKuduMasterHosts() { return defaultKuduMasterHosts_; } private void LibCacheSetNeedsRefresh(String hdfsLocation) { if (!FeSupport.NativeLibCacheSetNeedsRefresh(hdfsLocation)) { diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java index fb8bc4405..260da8810 100644 --- a/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java @@ -81,11 +81,9 @@ public class LocalCatalog implements FeCatalog { private Map<String, FeDb> dbs_ = new HashMap<>(); private Map<String, HdfsCachePool> hdfsCachePools_ = null; private String nullPartitionKeyValue_; - private final String defaultKuduMasterHosts_; - public LocalCatalog(MetaProvider metaProvider, String defaultKuduMasterHosts) { + public LocalCatalog(MetaProvider metaProvider) { metaProvider_ = Preconditions.checkNotNull(metaProvider); - defaultKuduMasterHosts_ = defaultKuduMasterHosts; } public String getProviderURI() { @@ -308,11 +306,6 @@ public class LocalCatalog implements FeCatalog { return metaProvider_.getAuthPolicy(); } - @Override - public String getDefaultKuduMasterHosts() { - return defaultKuduMasterHosts_; - } - public String getNullPartitionKeyValue() { if (nullPartitionKeyValue_ == null) { try { diff --git a/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java b/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java index ad639835a..e5ece72ce 100644 --- a/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java +++ b/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java @@ -49,8 +49,6 @@ import org.apache.thrift.TException; * from the catalogd via the statestore. */ public abstract class FeCatalogManager { - private static String DEFAULT_KUDU_MASTER_HOSTS = - BackendConfig.INSTANCE.getBackendCfg().kudu_master_hosts; protected AtomicReference<? extends AuthorizationChecker> authzChecker_; @@ -148,7 +146,7 @@ public abstract class FeCatalogManager { } private ImpaladCatalog createNewCatalog() { - return new ImpaladCatalog(DEFAULT_KUDU_MASTER_HOSTS, authzChecker_); + return new ImpaladCatalog(authzChecker_); } } @@ -163,7 +161,7 @@ public abstract class FeCatalogManager { @Override public FeCatalog getOrCreateCatalog() { PROVIDER.setAuthzChecker(authzChecker_); - return new LocalCatalog(PROVIDER, DEFAULT_KUDU_MASTER_HOSTS); + return new LocalCatalog(PROVIDER); } @Override @@ -189,7 +187,7 @@ public abstract class FeCatalogManager { throw new IllegalStateException("Create IcebergMetaProvider failed", e); } } - return new LocalCatalog(PROVIDER, null); + return new LocalCatalog(PROVIDER); } IcebergMetaProvider initProvider() throws IOException { diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java index 2ca9159ca..8c83feb54 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java @@ -2436,6 +2436,8 @@ public class AnalyzeDDLTest extends FrontendTestBase { "select double_col, int_col, tinyint_col from functional.alltypes", "Partition column name mismatch: tinyint_col != int_col"); + BackendConfig.INSTANCE.getBackendCfg().setKudu_master_hosts("127.0.0.1"); + // CTAS into managed Kudu tables AnalyzesOk("create table t primary key (id) partition by hash (id) partitions 3" + " stored as kudu as select id, bool_col, tinyint_col, smallint_col, int_col, " + diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java index bf8802726..276e09acc 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java @@ -19,9 +19,10 @@ package org.apache.impala.analysis; import org.apache.impala.catalog.KuduTable; import org.apache.impala.common.FrontendTestBase; -import org.apache.impala.testutil.TestUtils; +import org.apache.impala.service.BackendConfig; import org.apache.kudu.ColumnSchema.CompressionAlgorithm; import org.apache.kudu.ColumnSchema.Encoding; +import org.junit.BeforeClass; import org.junit.Test; import com.google.common.base.Joiner; @@ -34,6 +35,11 @@ import java.util.List; */ public class AnalyzeKuduDDLTest extends FrontendTestBase { + @BeforeClass + public static void setup() { + BackendConfig.INSTANCE.getBackendCfg().setKudu_master_hosts("127.0.0.1"); + } + /** * This is wrapper around super.AnalyzesOk method. The additional boolean is used to * add tblproperties for synchronized table @@ -319,7 +325,7 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase { "(PARTITION VALUE = 'abc')' is not a key column. Only key columns can be used " + "in PARTITION BY", isExternalPurgeTbl); // Kudu num_tablet_replicas is specified in tblproperties - String kuduMasters = catalog_.getDefaultKuduMasterHosts(); + String kuduMasters = BackendConfig.INSTANCE.getBackendCfg().kudu_master_hosts; AnalyzesOk(String.format("create table tab (x int primary key) partition by " + "hash (x) partitions 8 stored as kudu tblproperties " + "('kudu.num_tablet_replicas'='1', 'kudu.master_addresses' = '%s')", @@ -629,7 +635,7 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase { @Test public void TestCreateExternalKuduTable() { - final String kuduMasters = catalog_.getDefaultKuduMasterHosts(); + final String kuduMasters = BackendConfig.INSTANCE.getBackendCfg().kudu_master_hosts; AnalyzesOk("create external table t stored as kudu " + "tblproperties('kudu.table_name'='t')"); // Use all allowed optional table props. diff --git a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java index 0f891a515..cc1e43804 100644 --- a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java @@ -248,7 +248,7 @@ public class StmtMetadataLoaderTest { FeSupport.loadLibrary(); CatalogdMetaProvider provider = new CatalogdMetaProvider( BackendConfig.INSTANCE.getBackendCfg()); - LocalCatalog catalog = new LocalCatalog(provider, /*defaultKuduMasterHosts=*/null); + LocalCatalog catalog = new LocalCatalog(provider); Frontend fe = new Frontend(new NoopAuthorizationFactory(), catalog); EventSequence timeline = new EventSequence("Test Timeline"); User user = new User("user"); diff --git a/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java b/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java index 4ef81b33e..f32d4b806 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.fail; import org.apache.impala.authorization.Privilege; import org.apache.impala.common.AnalysisException; import org.apache.impala.common.FrontendTestBase; +import org.apache.impala.service.BackendConfig; import org.apache.impala.testutil.TestUtils; import org.junit.Test; @@ -352,7 +353,8 @@ public class ToSqlTest extends FrontendTestBase { "SORT BY ZORDER ( a, b ) STORED AS TEXTFILE" , true); // Kudu table with a TIMESTAMP column default value - String kuduMasters = catalog_.getDefaultKuduMasterHosts(); + String kuduMasters = "127.0.0.1"; + BackendConfig.INSTANCE.getBackendCfg().setKudu_master_hosts(kuduMasters); testToSql(String.format("create table p (a bigint primary key, " + "b timestamp default '1987-05-19') partition by hash(a) partitions 3 " + "stored as kudu tblproperties ('kudu.master_addresses'='%s')", kuduMasters), @@ -416,7 +418,8 @@ public class ToSqlTest extends FrontendTestBase { "( int_col, bool_col ) STORED AS TEXTFILE AS SELECT " + "int_col, bool_col, string_col FROM functional.alltypes", true); // Kudu table with multiple partition params - String kuduMasters = catalog_.getDefaultKuduMasterHosts(); + String kuduMasters = "127.0.0.1"; + BackendConfig.INSTANCE.getBackendCfg().setKudu_master_hosts(kuduMasters); testToSql(String.format("create table p primary key (a,b) " + "partition by hash(a) partitions 3, range (b) (partition value = 1) " + "stored as kudu tblproperties ('kudu.master_addresses'='%s') as select " + diff --git a/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java index e97b5a53b..f02af26fb 100644 --- a/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java +++ b/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java @@ -1910,6 +1910,7 @@ public class AuthorizationStmtTest extends AuthorizationTestBase { // ALL/OWNER privileges on SERVER are not required to create external Kudu tables // when 'kudu.master_addresses' is not specified as long as the RWSTORAGE privilege // is granted on the storage handler URI. + BackendConfig.INSTANCE.getBackendCfg().setKudu_master_hosts("127.0.0.1"); authorize("create external table functional.kudu_tbl stored as kudu " + "tblproperties ('kudu.table_name'='tbl')") .ok(onDatabase("functional", TPrivilegeLevel.ALL), diff --git a/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java b/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java index d0ff5836a..deb8bd647 100644 --- a/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java +++ b/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java @@ -74,7 +74,7 @@ public class LocalCatalogTest { public void setupCatalog() throws Exception { FeSupport.loadLibrary(); provider_ = new CatalogdMetaProvider(BackendConfig.INSTANCE.getBackendCfg()); - catalog_ = new LocalCatalog(provider_, /*defaultKuduMasterHosts=*/null); + catalog_ = new LocalCatalog(provider_); fe_ = new Frontend(new NoopAuthorizationFactory(), catalog_); } diff --git a/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java b/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java index ba04c0182..e8ac1b48b 100644 --- a/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java +++ b/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java @@ -18,6 +18,8 @@ package org.apache.impala.planner; import static org.junit.Assert.fail; + +import org.apache.impala.service.BackendConfig; import org.junit.Assert; import java.io.FileWriter; @@ -123,6 +125,7 @@ public class PlannerTestBase extends FrontendTestBase { String logDir = System.getenv("IMPALA_FE_TEST_LOGS_DIR"); if (logDir == null) logDir = "/tmp"; outDir_ = Paths.get(logDir, "PlannerTest"); + BackendConfig.INSTANCE.getBackendCfg().setKudu_master_hosts("127.0.0.1"); } @BeforeClass diff --git a/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java b/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java index 814e1cbc0..e9dc2bb24 100644 --- a/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java +++ b/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java @@ -57,7 +57,7 @@ public class ImpaladTestCatalog extends ImpaladCatalog { * Takes an {@link AuthorizationFactory} to bootstrap the backing CatalogServiceCatalog. */ public ImpaladTestCatalog(AuthorizationFactory authzFactory) { - super("127.0.0.1", null); + super(null); CatalogServiceCatalog catalogServerCatalog = CatalogServiceTestCatalog.createWithAuth(authzFactory); authPolicy_ = catalogServerCatalog.getAuthPolicy(); @@ -70,7 +70,7 @@ public class ImpaladTestCatalog extends ImpaladCatalog { * Creates ImpaladTestCatalog backed by a given catalog instance. */ public ImpaladTestCatalog(CatalogServiceCatalog catalog) { - super("127.0.0.1", null); + super(null); srcCatalog_ = Preconditions.checkNotNull(catalog); authPolicy_ = srcCatalog_.getAuthPolicy(); setIsReady(true);
