This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 228e9ed01c [fix](improvement)(meta) fix alter catalog properties
issues and reformat code (#14745)
228e9ed01c is described below
commit 228e9ed01c1e8550f728d01780c869b326613329
Author: Yulei-Yang <[email protected]>
AuthorDate: Fri Dec 2 09:34:13 2022 +0800
[fix](improvement)(meta) fix alter catalog properties issues and reformat
code (#14745)
1. fix NPE exception #14740
2. fix issue:
mysql> alter catalog xyz set properties
('hive.metastore.uris'='thrift://172.21.0.1:7004');
ERROR 1105 (HY000): errCode = 2, detailMessage = Can't modify the type of
catalog property with name: xyz
3. change behavior. The original logic is use props in set properties
clause to replace all exists props, now change to only replace the listed props
in set properties clause, and new props will be added. Make it behavior like
alter table property stmt.
---
.../apache/doris/common/util/PropertyAnalyzer.java | 2 +-
.../apache/doris/datasource/CatalogFactory.java | 2 +-
.../org/apache/doris/datasource/CatalogMgr.java | 13 +++++----
.../apache/doris/datasource/ExternalCatalog.java | 2 +-
.../doris/analysis/AlterCatalogNameStmtTest.java | 14 ++++-----
.../doris/analysis/AlterCatalogPropsStmtTest.java | 4 +--
.../apache/doris/datasource/CatalogMgrTest.java | 33 ++++++++++++++++------
7 files changed, 43 insertions(+), 27 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
index 034c7cea39..332cb3f5db 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
@@ -775,7 +775,7 @@ public class PropertyAnalyzer {
}
// validate the properties of es catalog
- if (properties.get("type").equalsIgnoreCase("es")) {
+ if ("es".equalsIgnoreCase(properties.get("type"))) {
try {
if (properties.containsKey(EsExternalCatalog.PROP_SSL)) {
EsUtil.getBoolean(properties, EsExternalCatalog.PROP_SSL);
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java
b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java
index da43241508..f5704e909d 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java
@@ -55,7 +55,7 @@ public class CatalogFactory {
} else if (stmt instanceof RefreshCatalogStmt) {
log.setCatalogId(catalogId);
} else {
- throw new RuntimeException("Unknown stmt for datasource manager "
+ stmt.getClass().getSimpleName());
+ throw new RuntimeException("Unknown stmt for catalog manager " +
stmt.getClass().getSimpleName());
}
return log;
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java
b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java
index 75e38f39b8..419bf71f24 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java
@@ -83,6 +83,11 @@ public class CatalogMgr implements Writable,
GsonPostProcessable {
initInternalCatalog();
}
+ public static CatalogMgr read(DataInput in) throws IOException {
+ String json = Text.readString(in);
+ return GsonUtils.GSON.fromJson(json, CatalogMgr.class);
+ }
+
private void initInternalCatalog() {
internalCatalog = new InternalCatalog();
addCatalog(internalCatalog);
@@ -267,7 +272,8 @@ public class CatalogMgr implements Writable,
GsonPostProcessable {
if (catalog == null) {
throw new DdlException("No catalog found with name: " +
stmt.getCatalogName());
}
- if
(!catalog.getType().equalsIgnoreCase(stmt.getNewProperties().get("type"))) {
+ if (stmt.getNewProperties().containsKey("type") &&
!catalog.getType()
+ .equalsIgnoreCase(stmt.getNewProperties().get("type"))) {
throw new DdlException("Can't modify the type of catalog
property with name: " + stmt.getCatalogName());
}
CatalogLog log =
CatalogFactory.constructorCatalogLog(catalog.getId(), stmt);
@@ -468,11 +474,6 @@ public class CatalogMgr implements Writable,
GsonPostProcessable {
Text.writeString(out, json);
}
- public static CatalogMgr read(DataInput in) throws IOException {
- String json = Text.readString(in);
- return GsonUtils.GSON.fromJson(json, CatalogMgr.class);
- }
-
@Override
public void gsonPostProcess() throws IOException {
for (CatalogIf catalog : idToCatalog.values()) {
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java
b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java
index 0eebe17298..2369e6afd4 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java
@@ -196,7 +196,7 @@ public abstract class ExternalCatalog implements
CatalogIf<ExternalDatabase>, Wr
@Override
public void modifyCatalogProps(Map<String, String> props) {
- catalogProperty.setProperties(props);
+ catalogProperty.getProperties().putAll(props);
}
@Override
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogNameStmtTest.java
b/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogNameStmtTest.java
index d2b9f6a687..44188e2852 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogNameStmtTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogNameStmtTest.java
@@ -50,40 +50,40 @@ public class AlterCatalogNameStmtTest {
public void testNormalCase() throws UserException {
AlterCatalogNameStmt stmt = new AlterCatalogNameStmt("testCatalog",
"testNewCatalog");
stmt.analyze(analyzer);
- Assert.assertEquals("testCatalog", stmt.getCatalogName());
- Assert.assertEquals("testNewCatalog", stmt.getNewCatalogName());
+ Assert.assertEquals("testCatalog", stmt.getCatalogName());
+ Assert.assertEquals("testNewCatalog", stmt.getNewCatalogName());
}
@Test(expected = AnalysisException.class)
- public void testEmptyDs1() throws UserException {
+ public void testEmptyDs1() throws UserException {
AlterCatalogNameStmt stmt = new AlterCatalogNameStmt("",
"testNewCatalog");
stmt.analyze(analyzer);
Assert.fail("No exception throws.");
}
@Test(expected = AnalysisException.class)
- public void testEmptyDs2() throws UserException {
+ public void testEmptyDs2() throws UserException {
AlterCatalogNameStmt stmt = new AlterCatalogNameStmt("testCatalog",
"");
stmt.analyze(analyzer);
Assert.fail("No exception throws.");
}
@Test(expected = AnalysisException.class)
- public void testBuildIn1() throws UserException {
+ public void testBuildIn1() throws UserException {
AlterCatalogNameStmt stmt = new
AlterCatalogNameStmt(InternalCatalog.INTERNAL_CATALOG_NAME, "testNewCatalog");
stmt.analyze(analyzer);
Assert.fail("No exception throws.");
}
@Test(expected = AnalysisException.class)
- public void testBuildIn2() throws UserException {
+ public void testBuildIn2() throws UserException {
AlterCatalogNameStmt stmt = new AlterCatalogNameStmt("testCatalog",
InternalCatalog.INTERNAL_CATALOG_NAME);
stmt.analyze(analyzer);
Assert.fail("No exception throws.");
}
@Test(expected = AnalysisException.class)
- public void testNameFormat() throws UserException {
+ public void testNameFormat() throws UserException {
AlterCatalogNameStmt stmt = new AlterCatalogNameStmt("testCatalog",
InternalCatalog.INTERNAL_CATALOG_NAME);
stmt.analyze(analyzer);
Assert.fail("No exception throws.");
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogPropsStmtTest.java
b/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogPropsStmtTest.java
index f5cc841f34..08d123c771 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogPropsStmtTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/analysis/AlterCatalogPropsStmtTest.java
@@ -56,8 +56,8 @@ public class AlterCatalogPropsStmtTest {
props.put("hive.metastore.uris", "thrift://localhost:9083");
AlterCatalogPropertyStmt stmt = new
AlterCatalogPropertyStmt("testCatalog", props);
stmt.analyze(analyzer);
- Assert.assertEquals("testCatalog", stmt.getCatalogName());
- Assert.assertEquals(2, stmt.getNewProperties().size());
+ Assert.assertEquals("testCatalog", stmt.getCatalogName());
+ Assert.assertEquals(2, stmt.getNewProperties().size());
}
@Test(expected = AnalysisException.class)
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java
b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java
index 781d94b491..647d5ee690 100644
--- a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java
+++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java
@@ -44,6 +44,7 @@ import org.apache.doris.system.SystemInfoService;
import org.apache.doris.utframe.TestWithFeService;
import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
import org.junit.Assert;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -57,13 +58,12 @@ import java.util.List;
import java.util.Map;
public class CatalogMgrTest extends TestWithFeService {
- private CatalogMgr mgr;
private static final String MY_CATALOG = "my_catalog";
-
private static PaloAuth auth;
private static Env env;
private static UserIdentity user1;
private static UserIdentity user2;
+ private CatalogMgr mgr;
@Override
protected void runBeforeAll() throws Exception {
@@ -171,23 +171,37 @@ public class CatalogMgrTest extends TestWithFeService {
AlterCatalogNameStmt alterNameStmt = (AlterCatalogNameStmt)
parseAndAnalyzeStmt(alterCatalogNameSql);
mgr.alterCatalogName(alterNameStmt);
+ // test modify property
String alterCatalogProps = "ALTER CATALOG " + MY_CATALOG + " SET
PROPERTIES"
- + " (\"type\" = \"hms\", \"k\" = \"v\");";
+ + " (\"type\" = \"hms\", \"hive.metastore.uris\" =
\"thrift://172.16.5.9:9083\");";
AlterCatalogPropertyStmt alterPropStmt = (AlterCatalogPropertyStmt)
parseAndAnalyzeStmt(alterCatalogProps);
mgr.alterCatalogProps(alterPropStmt);
+ CatalogIf catalog = env.getCatalogMgr().getCatalog(MY_CATALOG);
+ Assert.assertEquals(2, catalog.getProperties().size());
+ Assert.assertEquals("thrift://172.16.5.9:9083",
catalog.getProperties().get("hive.metastore.uris"));
+
+ // test add property
+ Map<String, String> alterProps2 = Maps.newHashMap();
+ alterProps2.put("dfs.nameservices", "service1");
+ alterProps2.put("dfs.ha.namenodes.service1", "nn1,nn2");
+ AlterCatalogPropertyStmt alterStmt = new
AlterCatalogPropertyStmt(MY_CATALOG, alterProps2);
+ mgr.alterCatalogProps(alterStmt);
+ catalog = env.getCatalogMgr().getCatalog(MY_CATALOG);
+ Assert.assertEquals(4, catalog.getProperties().size());
+ Assert.assertEquals("service1",
catalog.getProperties().get("dfs.nameservices"));
+
String showDetailCatalog = "SHOW CATALOG my_catalog";
ShowCatalogStmt showDetailStmt = (ShowCatalogStmt)
parseAndAnalyzeStmt(showDetailCatalog);
showResultSet = mgr.showCatalogs(showDetailStmt);
+ Assert.assertEquals(4, showResultSet.getResultRows().size());
for (List<String> row : showResultSet.getResultRows()) {
Assertions.assertEquals(2, row.size());
if (row.get(0).equalsIgnoreCase("type")) {
Assertions.assertEquals("hms", row.get(1));
- } else if (row.get(0).equalsIgnoreCase("k")) {
- Assertions.assertEquals("v", row.get(1));
- } else {
- Assertions.fail();
+ } else if
(row.get(0).equalsIgnoreCase("dfs.ha.namenodes.service1")) {
+ Assertions.assertEquals("nn1,nn2", row.get(1));
}
}
@@ -238,9 +252,9 @@ public class CatalogMgrTest extends TestWithFeService {
CatalogIf hms = mgr2.getCatalog(MY_CATALOG);
properties = hms.getProperties();
- Assert.assertEquals(2, properties.size());
+ Assert.assertEquals(4, properties.size());
Assert.assertEquals("hms", properties.get("type"));
- Assert.assertEquals("v", properties.get("k"));
+ Assert.assertEquals("thrift://172.16.5.9:9083",
properties.get("hive.metastore.uris"));
// 3. delete files
dis.close();
@@ -322,4 +336,5 @@ public class CatalogMgrTest extends TestWithFeService {
"errCode = 2, detailMessage = Access denied for user
'default_cluster:user2' to catalog 'iceberg'");
}
}
+
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
