From eac8161ac1f125533986a71cc0059b1f3a918b86 Mon Sep 17 00:00:00 2001 From: wangbo <506340561@qq.com> Date: Thu, 18 Feb 2021 16:01:53 +0800 Subject: [PATCH 1/4] (#5390)fix NPE when replay colocate group --- .../org/apache/doris/catalog/Catalog.java | 43 +++++++++++-------- .../org/apache/doris/catalog/Database.java | 8 +++- .../apache/doris/catalog/InfoSchemaDb.java | 5 ++- .../doris/catalog/InfoSchemaDbTest.java | 2 +- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java index 7ea431f3bf1cac..5bbaa49fe88971 100755 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java @@ -3769,22 +3769,27 @@ private void createOlapTable(Database db, CreateTableStmt stmt) throws DdlExcept throw new DdlException("Unsupported partition method: " + partitionInfo.getType().name()); } - if (!db.createTableWithLock(olapTable, false, stmt.isSetIfNotExists())) { + Pair result = db.createTableWithLock(olapTable, false, stmt.isSetIfNotExists()); + if (!result.first) { ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, tableName, "table already exists"); } - // we have added these index to memory, only need to persist here - if (getColocateTableIndex().isColocateTable(tableId)) { - GroupId groupId = getColocateTableIndex().getGroup(tableId); - List> backendsPerBucketSeq = getColocateTableIndex().getBackendsPerBucketSeq(groupId); - ColocatePersistInfo info = ColocatePersistInfo.createForAddTable(groupId, tableId, backendsPerBucketSeq); - editLog.logColocateAddTable(info); - } - LOG.info("successfully create table[{};{}]", tableName, tableId); - // register or remove table from DynamicPartition after table created - DynamicPartitionUtil.registerOrRemoveDynamicPartitionTable(db.getId(), olapTable, false); - dynamicPartitionScheduler.createOrUpdateRuntimeInfo( - tableName, DynamicPartitionScheduler.LAST_UPDATE_TIME, TimeUtils.getCurrentFormatTime()); + if (result.second) { + LOG.info("duplicate create table[{};{}], skip next steps", tableName, tableId); + } else { + // we have added these index to memory, only need to persist here + if (getColocateTableIndex().isColocateTable(tableId)) { + GroupId groupId = getColocateTableIndex().getGroup(tableId); + List> backendsPerBucketSeq = getColocateTableIndex().getBackendsPerBucketSeq(groupId); + ColocatePersistInfo info = ColocatePersistInfo.createForAddTable(groupId, tableId, backendsPerBucketSeq); + editLog.logColocateAddTable(info); + } + LOG.info("successfully create table[{};{}]", tableName, tableId); + // register or remove table from DynamicPartition after table created + DynamicPartitionUtil.registerOrRemoveDynamicPartitionTable(db.getId(), olapTable, false); + dynamicPartitionScheduler.createOrUpdateRuntimeInfo( + tableName, DynamicPartitionScheduler.LAST_UPDATE_TIME, TimeUtils.getCurrentFormatTime()); + } } catch (DdlException e) { for (Long tabletId : tabletIdSet) { Catalog.getCurrentInvertedIndex().deleteTablet(tabletId); @@ -3807,7 +3812,7 @@ private void createMysqlTable(Database db, CreateTableStmt stmt) throws DdlExcep long tableId = Catalog.getCurrentCatalog().getNextId(); MysqlTable mysqlTable = new MysqlTable(tableId, tableName, columns, stmt.getProperties()); mysqlTable.setComment(stmt.getComment()); - if (!db.createTableWithLock(mysqlTable, false, stmt.isSetIfNotExists())) { + if (!db.createTableWithLock(mysqlTable, false, stmt.isSetIfNotExists()).first) { ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, tableName, "table already exist"); } LOG.info("successfully create table[{}-{}]", tableName, tableId); @@ -3822,7 +3827,7 @@ private void createOdbcTable(Database db, CreateTableStmt stmt) throws DdlExcept long tableId = Catalog.getCurrentCatalog().getNextId(); OdbcTable odbcTable = new OdbcTable(tableId, tableName, columns, stmt.getProperties()); odbcTable.setComment(stmt.getComment()); - if (!db.createTableWithLock(odbcTable, false, stmt.isSetIfNotExists())) { + if (!db.createTableWithLock(odbcTable, false, stmt.isSetIfNotExists()).first) { ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, tableName, "table already exist"); } LOG.info("successfully create table[{}-{}]", tableName, tableId); @@ -3853,7 +3858,7 @@ private Table createEsTable(Database db, CreateTableStmt stmt) throws DdlExcepti EsTable esTable = new EsTable(tableId, tableName, baseSchema, stmt.getProperties(), partitionInfo); esTable.setComment(stmt.getComment()); - if (!db.createTableWithLock(esTable, false, stmt.isSetIfNotExists())) { + if (!db.createTableWithLock(esTable, false, stmt.isSetIfNotExists()).first) { ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, tableName, "table already exist"); } LOG.info("successfully create table{} with id {}", tableName, tableId); @@ -3870,7 +3875,7 @@ private void createBrokerTable(Database db, CreateTableStmt stmt) throws DdlExce brokerTable.setComment(stmt.getComment()); brokerTable.setBrokerProperties(stmt.getExtProperties()); - if (!db.createTableWithLock(brokerTable, false, stmt.isSetIfNotExists())) { + if (!db.createTableWithLock(brokerTable, false, stmt.isSetIfNotExists()).first) { ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, tableName, "table already exist"); } LOG.info("successfully create table[{}-{}]", tableName, tableId); @@ -3884,7 +3889,7 @@ private void createHiveTable(Database db, CreateTableStmt stmt) throws DdlExcept long tableId = getNextId(); HiveTable hiveTable = new HiveTable(tableId, tableName, columns, stmt.getProperties()); hiveTable.setComment(stmt.getComment()); - if (!db.createTableWithLock(hiveTable, false, stmt.isSetIfNotExists())) { + if (!db.createTableWithLock(hiveTable, false, stmt.isSetIfNotExists()).first) { ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, tableName, "table already exist"); } LOG.info("successfully create table[{}-{}]", tableName, tableId); @@ -5531,7 +5536,7 @@ public void createView(CreateViewStmt stmt) throws DdlException { throw new DdlException("failed to init view stmt", e); } - if (!db.createTableWithLock(newView, false, stmt.isSetIfNotExists())) { + if (!db.createTableWithLock(newView, false, stmt.isSetIfNotExists()).first) { throw new DdlException("Failed to create view[" + tableName + "]."); } LOG.info("successfully create view[" + tableName + "-" + newView.getId() + "]"); diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java index 202c8eb0720a8c..c1d6f975d36c59 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java @@ -281,13 +281,17 @@ public void checkQuota() throws DdlException { checkReplicaQuota(); } - public boolean createTableWithLock(Table table, boolean isReplay, boolean setIfNotExist) { + public Pair createTableWithLock(Table table, boolean isReplay, boolean setIfNotExist) { boolean result = true; + // if a table is already exists, then edit log won't be executed + // some caller of this method may need to know this message + boolean isTableExist = false; writeLock(); try { String tableName = table.getName(); if (nameToTable.containsKey(tableName)) { result = setIfNotExist; + isTableExist = true; } else { idToTable.put(table.getId(), table); nameToTable.put(table.getName(), table); @@ -301,7 +305,7 @@ public boolean createTableWithLock(Table table, boolean isReplay, boolean setIfN Catalog.getCurrentCatalog().getEsRepository().registerTable((EsTable)table); } } - return result; + return Pair.create(result, isTableExist); } finally { writeUnlock(); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/InfoSchemaDb.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/InfoSchemaDb.java index e577d4f256b351..78666f64e586d8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/InfoSchemaDb.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/InfoSchemaDb.java @@ -18,6 +18,7 @@ package org.apache.doris.catalog; import org.apache.doris.cluster.ClusterNamespace; +import org.apache.doris.common.Pair; import org.apache.doris.common.SystemIdGenerator; import java.io.DataInput; @@ -39,8 +40,8 @@ public InfoSchemaDb(String cluster) { } @Override - public boolean createTableWithLock(Table table, boolean isReplay, boolean setIfNotExist) { - return false; + public Pair createTableWithLock(Table table, boolean isReplay, boolean setIfNotExist) { + return Pair.create(false, false); } @Override diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/InfoSchemaDbTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/InfoSchemaDbTest.java index 08a47a8ab1805d..1cf7fdd338ae20 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/InfoSchemaDbTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/InfoSchemaDbTest.java @@ -28,7 +28,7 @@ public void testNormal() throws IOException { Database db = new InfoSchemaDb(); Assert.assertFalse(db.createTable(null)); - Assert.assertFalse(db.createTableWithLock(null, false, false)); + Assert.assertFalse(db.createTableWithLock(null, false, false).first); db.dropTable("authors"); db.write(null); Assert.assertNull(db.getTable("authors")); From 773d0a7c889c7348b4a2a998e3c47ad6673db02d Mon Sep 17 00:00:00 2001 From: wangbo <506340561@qq.com> Date: Wed, 3 Mar 2021 20:36:15 +0800 Subject: [PATCH 2/4] remove table id from colocate group when duplicate create table --- .../src/main/java/org/apache/doris/catalog/Catalog.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java index 5bbaa49fe88971..42e632dcc4a865 100755 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java @@ -3775,6 +3775,11 @@ private void createOlapTable(Database db, CreateTableStmt stmt) throws DdlExcept } if (result.second) { + if (getColocateTableIndex().isColocateTable(tableId)) { + // if this is a colocate join table, its table id is already added to colocate group + // so we should remove the tableId here + getColocateTableIndex().removeTable(tableId); + } LOG.info("duplicate create table[{};{}], skip next steps", tableName, tableId); } else { // we have added these index to memory, only need to persist here From 4a548261703a9f41b4a8c09d0565650cded425ec Mon Sep 17 00:00:00 2001 From: wangbo <506340561@qq.com> Date: Mon, 8 Mar 2021 11:10:03 +0800 Subject: [PATCH 3/4] remove tablet id when duplicate create table,just like ddlexception --- fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java index 42e632dcc4a865..d2d67343ffbe08 100755 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java @@ -3780,6 +3780,9 @@ private void createOlapTable(Database db, CreateTableStmt stmt) throws DdlExcept // so we should remove the tableId here getColocateTableIndex().removeTable(tableId); } + for (Long tabletId : tabletIdSet) { + Catalog.getCurrentInvertedIndex().deleteTablet(tabletId); + } LOG.info("duplicate create table[{};{}], skip next steps", tableName, tableId); } else { // we have added these index to memory, only need to persist here From 6dfe9e9222d7eac909f99565e422ae40c92a03dc Mon Sep 17 00:00:00 2001 From: wangbo <506340561@qq.com> Date: Wed, 10 Mar 2021 15:06:17 +0800 Subject: [PATCH 4/4] add ut --- .../doris/catalog/ColocateTableIndex.java | 6 ++++ .../doris/catalog/TabletInvertedIndex.java | 20 +++++++++++ .../apache/doris/catalog/CreateTableTest.java | 34 +++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColocateTableIndex.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColocateTableIndex.java index 7c5fd98b8dc1e0..dc6bf25820967b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColocateTableIndex.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColocateTableIndex.java @@ -700,4 +700,10 @@ public void setBackendsSetByIdxForGroup(GroupId groupId, int tabletOrderIdx, Set writeUnlock(); } } + + // just for ut + public Map getTable2Group() { + return table2Group; + } + } diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/TabletInvertedIndex.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/TabletInvertedIndex.java index edad1e0e6e81cc..3e424e79d21840 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/TabletInvertedIndex.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/TabletInvertedIndex.java @@ -695,5 +695,25 @@ public PartitionBalanceInfo(PartitionBalanceInfo info) { this.beByReplicaCount = TreeMultimap.create(info.beByReplicaCount); } } + + // just for ut + public Table getReplicaMetaTable() { + return replicaMetaTable; + } + + // just for ut + public Table getBackingReplicaMetaTable() { + return backingReplicaMetaTable; + } + + // just for ut + public Table getTabletMetaTable() { + return tabletMetaTable; + } + + // just for ut + public Map getTabletMetaMap() { + return tabletMetaMap; + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java index dab5fbd4af4a9d..6bfdb879b41cfc 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java @@ -32,6 +32,8 @@ import org.junit.Test; import java.io.File; +import java.util.HashSet; +import java.util.Set; import java.util.UUID; public class CreateTableTest { @@ -62,6 +64,38 @@ private static void createTable(String sql) throws Exception { Catalog.getCurrentCatalog().createTable(createTableStmt); } + @Test + public void testDuplicateCreateTable() throws Exception{ + // test + Catalog catalog = Catalog.getCurrentCatalog(); + String sql = "create table if not exists test.tbl1\n" + "(k1 int, k2 int)\n" + "duplicate key(k1)\n" + + "distributed by hash(k2) buckets 1\n" + "properties('replication_num' = '1','colocate_with'='test'); "; + createTable(sql); + Set tabletIdSetAfterCreateFirstTable = catalog.getTabletInvertedIndex().getReplicaMetaTable().rowKeySet(); + Set tabletMetaSetBeforeCreateFirstTable = new HashSet<>(); + catalog.getTabletInvertedIndex().getTabletMetaTable().values().forEach(tabletMeta -> {tabletMetaSetBeforeCreateFirstTable.add(tabletMeta);}); + Set colocateTableIdBeforeCreateFirstTable = catalog.getColocateTableIndex().getTable2Group().keySet(); + Assert.assertTrue(colocateTableIdBeforeCreateFirstTable.size() > 0); + Assert.assertTrue(tabletIdSetAfterCreateFirstTable.size() > 0); + + createTable(sql); + // check whether tablet is cleared after duplicate create table + Set tabletIdSetAfterDuplicateCreateTable1 = catalog.getTabletInvertedIndex().getReplicaMetaTable().rowKeySet(); + Set tabletIdSetAfterDuplicateCreateTable2 = catalog.getTabletInvertedIndex().getBackingReplicaMetaTable().columnKeySet(); + Set tabletIdSetAfterDuplicateCreateTable3 = catalog.getTabletInvertedIndex().getTabletMetaMap().keySet(); + Set tabletIdSetAfterDuplicateCreateTable4 = new HashSet<>(); + catalog.getTabletInvertedIndex().getTabletMetaTable().values().forEach(tabletMeta -> {tabletIdSetAfterDuplicateCreateTable4.add(tabletMeta);}); + + Assert.assertTrue(tabletIdSetAfterCreateFirstTable.equals(tabletIdSetAfterDuplicateCreateTable1)); + Assert.assertTrue(tabletIdSetAfterCreateFirstTable.equals(tabletIdSetAfterDuplicateCreateTable2)); + Assert.assertTrue(tabletIdSetAfterCreateFirstTable.equals(tabletIdSetAfterDuplicateCreateTable3)); + Assert.assertTrue(tabletMetaSetBeforeCreateFirstTable.equals(tabletIdSetAfterDuplicateCreateTable4)); + + // check whether table id is cleared from colocate group after duplicate create table + Set colocateTableIdAfterCreateFirstTable = catalog.getColocateTableIndex().getTable2Group().keySet(); + Assert.assertTrue(colocateTableIdBeforeCreateFirstTable.equals(colocateTableIdAfterCreateFirstTable)); + } + @Test public void testNormal() throws DdlException { ExceptionChecker.expectThrowsNoException(