This is an automated email from the ASF dual-hosted git repository.
yiguolei 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 8162d0062b [fix](alter) fix potential concurrent issue for alter when
check olap table state normal outside write lock scope is not atomic (#20480)
8162d0062b is described below
commit 8162d0062bc42435aaa0145d7a3ebe4629744a55
Author: caiconghui <55968745+caicong...@users.noreply.github.com>
AuthorDate: Sun Jun 11 18:17:41 2023 +0800
[fix](alter) fix potential concurrent issue for alter when check olap table
state normal outside write lock scope is not atomic (#20480)
now, we check some olap table state normal outside write lock scope, the
table state may be changed to unnormal when we do alter operation
---------
Co-authored-by: caiconghui1 <caicongh...@jd.com>
---
.../main/java/org/apache/doris/alter/Alter.java | 14 ++------
.../doris/alter/MaterializedViewHandler.java | 13 +++-----
.../apache/doris/alter/SchemaChangeHandler.java | 14 +-------
.../main/java/org/apache/doris/catalog/Env.java | 20 +++---------
.../java/org/apache/doris/catalog/OlapTable.java | 18 ++---------
.../apache/doris/datasource/InternalCatalog.java | 37 ++++++----------------
.../apache/doris/service/FrontendServiceImpl.java | 1 +
.../doris/alter/MaterializedViewHandlerTest.java | 2 --
8 files changed, 25 insertions(+), 94 deletions(-)
diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
index b89cb0b733..ef018b04d2 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
@@ -52,7 +52,6 @@ import org.apache.doris.catalog.MaterializedView;
import org.apache.doris.catalog.MysqlTable;
import org.apache.doris.catalog.OdbcTable;
import org.apache.doris.catalog.OlapTable;
-import org.apache.doris.catalog.OlapTable.OlapTableState;
import org.apache.doris.catalog.Partition;
import org.apache.doris.catalog.PartitionInfo;
import org.apache.doris.catalog.Replica;
@@ -189,11 +188,7 @@ public class Alter {
db.checkQuota();
}
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException(
- "Table[" + olapTable.getName() + "]'s state is not NORMAL.
Do not allow doing ALTER ops");
- }
-
+ olapTable.checkNormalStateForAlter();
boolean needProcessOutsideTableLock = false;
if (currentAlterOps.checkTableStoragePolicy(alterClauses)) {
String tableStoragePolicy = olapTable.getStoragePolicy();
@@ -256,7 +251,7 @@ public class Alter {
if
(properties.containsKey(PropertyAnalyzer.PROPERTIES_INMEMORY)) {
boolean isInMemory =
Boolean.parseBoolean(properties.get(PropertyAnalyzer.PROPERTIES_INMEMORY));
- if (isInMemory == true) {
+ if (isInMemory) {
throw new UserException("Not support set
'in_memory'='true' now!");
}
needProcessOutsideTableLock = true;
@@ -780,10 +775,7 @@ public class Alter {
throws DdlException, AnalysisException {
Preconditions.checkArgument(olapTable.isWriteLockHeldByCurrentThread());
List<ModifyPartitionInfo> modifyPartitionInfos = Lists.newArrayList();
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + olapTable.getName() + "]'s state
is not NORMAL");
- }
-
+ olapTable.checkNormalStateForAlter();
for (String partitionName : partitionNames) {
Partition partition = olapTable.getPartition(partitionName,
isTempPartition);
if (partition == null) {
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
b/fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
index e54c18a357..baf0fd60f3 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
@@ -185,7 +185,7 @@ public class MaterializedViewHandler extends AlterHandler {
throws DdlException, AnalysisException {
olapTable.writeLockOrDdlException();
try {
- olapTable.checkStableAndNormal(db.getClusterName());
+ olapTable.checkNormalStateForAlter();
if (olapTable.existTempPartitions()) {
throw new DdlException("Can not alter table when there are
temp partitions in table");
}
@@ -244,6 +244,7 @@ public class MaterializedViewHandler extends AlterHandler {
Set<Long> logJobIdSet = new HashSet<>();
olapTable.writeLockOrDdlException();
try {
+ olapTable.checkNormalStateForAlter();
if (olapTable.existTempPartitions()) {
throw new DdlException("Can not alter table when there are
temp partitions in table");
}
@@ -874,6 +875,7 @@ public class MaterializedViewHandler extends AlterHandler {
throws DdlException, MetaNotFoundException {
olapTable.writeLockOrDdlException();
try {
+ olapTable.checkNormalStateForAlter();
if (olapTable.existTempPartitions()) {
throw new DdlException("Can not alter table when there are
temp partitions in table");
}
@@ -911,12 +913,7 @@ public class MaterializedViewHandler extends AlterHandler {
OlapTable olapTable) throws DdlException, MetaNotFoundException {
olapTable.writeLockOrDdlException();
try {
- // check table state
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + olapTable.getName() + "]'s
state is not NORMAL. "
- + "Do not allow doing DROP ops");
- }
-
+ olapTable.checkNormalStateForAlter();
String mvName = dropMaterializedViewStmt.getMvName();
// Step1: check drop mv index operation
checkDropMaterializedView(mvName, olapTable);
@@ -946,7 +943,6 @@ public class MaterializedViewHandler extends AlterHandler {
*/
private void checkDropMaterializedView(String mvName, OlapTable olapTable)
throws DdlException, MetaNotFoundException {
- Preconditions.checkState(olapTable.getState() ==
OlapTableState.NORMAL, olapTable.getState().name());
if (mvName.equals(olapTable.getName())) {
throw new DdlException("Cannot drop base index by using DROP
ROLLUP or DROP MATERIALIZED VIEW.");
}
@@ -1275,7 +1271,6 @@ public class MaterializedViewHandler extends AlterHandler
{
onJobDone(alterJobV2);
}
}
- return;
}
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
index 0a06205963..8075443439 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
@@ -1173,13 +1173,7 @@ public class SchemaChangeHandler extends AlterHandler {
private void createJob(long dbId, OlapTable olapTable, Map<Long,
LinkedList<Column>> indexSchemaMap,
Map<String, String> propertyMap, List<Index>
indexes) throws UserException {
- if (olapTable.getState() == OlapTableState.ROLLUP) {
- throw new DdlException("Table[" + olapTable.getName() + "]'s is
doing ROLLUP job");
- }
-
checkReplicaCount(olapTable);
- // for now table's state can only be NORMAL
- Preconditions.checkState(olapTable.getState() ==
OlapTableState.NORMAL, olapTable.getState().name());
// process properties first
// for now. properties has 3 options
@@ -1751,6 +1745,7 @@ public class SchemaChangeHandler extends AlterHandler {
throws UserException {
olapTable.writeLockOrDdlException();
try {
+ olapTable.checkNormalStateForAlter();
//alterClauses can or cannot light schema change
boolean lightSchemaChange = true;
boolean lightIndexChange = false;
@@ -2471,13 +2466,6 @@ public class SchemaChangeHandler extends AlterHandler {
throws DdlException {
LOG.debug("indexSchemaMap:{}, indexes:{}", indexSchemaMap, indexes);
- if (olapTable.getState() == OlapTableState.ROLLUP) {
- throw new DdlException("Table[" + olapTable.getName() + "]'s is
doing ROLLUP job");
- }
-
- // for now table's state can only be NORMAL
- Preconditions.checkState(olapTable.getState() ==
OlapTableState.NORMAL, olapTable.getState().name());
-
// for bitmapIndex
boolean hasIndexChange = false;
Set<Index> newSet = new HashSet<>(indexes);
diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java
b/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java
index 167acbb86d..b853232f42 100755
--- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java
@@ -85,7 +85,6 @@ import org.apache.doris.catalog.ColocateTableIndex.GroupId;
import org.apache.doris.catalog.DistributionInfo.DistributionInfoType;
import org.apache.doris.catalog.MaterializedIndex.IndexExtState;
import org.apache.doris.catalog.MetaIdGenerator.IdGeneratorBuffer;
-import org.apache.doris.catalog.OlapTable.OlapTableState;
import org.apache.doris.catalog.Replica.ReplicaStatus;
import org.apache.doris.catalog.TableIf.TableType;
import org.apache.doris.clone.ColocateTableCheckerAndBalancer;
@@ -3919,9 +3918,7 @@ public class Env {
try {
if (table instanceof OlapTable) {
OlapTable olapTable = (OlapTable) table;
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + olapTable.getName()
+ "] is under " + olapTable.getState());
- }
+ olapTable.checkNormalStateForAlter();
}
String oldTableName = table.getName();
@@ -4104,10 +4101,7 @@ public class Env {
public void renameRollup(Database db, OlapTable table, RollupRenameClause
renameClause) throws DdlException {
table.writeLockOrDdlException();
try {
- if (table.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + table.getName() + "] is
under " + table.getState());
- }
-
+ table.checkNormalStateForAlter();
String rollupName = renameClause.getRollupName();
// check if it is base table name
if (rollupName.equals(table.getName())) {
@@ -4165,10 +4159,7 @@ public class Env {
public void renamePartition(Database db, OlapTable table,
PartitionRenameClause renameClause) throws DdlException {
table.writeLockOrDdlException();
try {
- if (table.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + table.getName() + "] is
under " + table.getState());
- }
-
+ table.checkNormalStateForAlter();
if (table.getPartitionInfo().getType() != PartitionType.RANGE
&& table.getPartitionInfo().getType() !=
PartitionType.LIST) {
throw new DdlException(
@@ -4223,10 +4214,7 @@ public class Env {
private void renameColumn(Database db, OlapTable table, String colName,
String newColName, boolean isReplay) throws
DdlException {
- if (table.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + table.getName() + "] is under "
+ table.getState());
- }
-
+ table.checkNormalStateForAlter();
if (colName.equalsIgnoreCase(newColName)) {
throw new DdlException("Same column name");
}
diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java
b/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java
index 91910331e8..af4a653f46 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java
@@ -1498,21 +1498,9 @@ public class OlapTable extends Table {
return replicaCount;
}
- public void checkStableAndNormal(String clusterName) throws DdlException {
+ public void checkNormalStateForAlter() throws DdlException {
if (state != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + name + "]'s state is not NORMAL.
"
- + "Do not allow doing materialized view");
- }
- // check if all tablets are healthy, and no tablet is in tablet
scheduler
- boolean isStable = isStable(Env.getCurrentSystemInfo(),
- Env.getCurrentEnv().getTabletScheduler(),
- clusterName);
- if (!isStable) {
- throw new DdlException("table [" + name + "] is not stable."
- + " Some tablets of this table may not be healthy or are
being "
- + "scheduled."
- + " You need to repair the table first"
- + " or stop cluster balance. See 'help admin;'.");
+ throw new DdlException("Table[" + name + "]'s state is not NORMAL.
Do not allow doing ALTER ops");
}
}
@@ -1776,7 +1764,7 @@ public class OlapTable extends Table {
}
public boolean isCcrEnable() {
- return tableProperty != null ? tableProperty.isCcrEnable() : false;
+ return tableProperty != null && tableProperty.isCcrEnable();
}
public void setDisableAutoCompaction(boolean disableAutoCompaction) {
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
b/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
index 1830a2b36d..24e4b3ec09 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
@@ -1344,16 +1344,10 @@ public class InternalCatalog implements
CatalogIf<Database> {
String partitionName = singlePartitionDesc.getPartitionName();
// check
- Table table = db.getOlapTableOrDdlException(tableName);
- // check state
- OlapTable olapTable = (OlapTable) table;
-
+ OlapTable olapTable = db.getOlapTableOrDdlException(tableName);
olapTable.readLock();
try {
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + tableName + "]'s state is
not NORMAL");
- }
-
+ olapTable.checkNormalStateForAlter();
// check partition type
PartitionInfo partitionInfo = olapTable.getPartitionInfo();
if (partitionInfo.getType() != PartitionType.RANGE &&
partitionInfo.getType() != PartitionType.LIST) {
@@ -1512,14 +1506,10 @@ public class InternalCatalog implements
CatalogIf<Database> {
olapTable.skipWriteIndexOnLoad(),
olapTable.storeRowColumn(), olapTable.isDynamicSchema());
// check again
- table = db.getOlapTableOrDdlException(tableName);
- table.writeLockOrDdlException();
+ olapTable = db.getOlapTableOrDdlException(tableName);
+ olapTable.writeLockOrDdlException();
try {
- olapTable = (OlapTable) table;
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + tableName + "]'s state
is not NORMAL");
- }
-
+ olapTable.checkNormalStateForAlter();
// check partition name
if (olapTable.checkPartitionNameExist(partitionName)) {
if (singlePartitionDesc.isSetIfNotExists()) {
@@ -1587,7 +1577,7 @@ public class InternalCatalog implements
CatalogIf<Database> {
LOG.info("succeed in creating partition[{}], temp: {}",
partitionId, isTempPartition);
} finally {
- table.writeUnlock();
+ olapTable.writeUnlock();
}
} catch (DdlException e) {
for (Long tabletId : tabletIdSet) {
@@ -1648,10 +1638,7 @@ public class InternalCatalog implements
CatalogIf<Database> {
String partitionName = clause.getPartitionName();
boolean isTempPartition = clause.isTempPartition();
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + olapTable.getName() + "]'s state
is not NORMAL");
- }
-
+ olapTable.checkNormalStateForAlter();
if (!olapTable.checkPartitionNameExist(partitionName,
isTempPartition)) {
if (clause.isSetIfExists()) {
LOG.info("drop partition[{}] which does not exist",
partitionName);
@@ -2692,10 +2679,7 @@ public class InternalCatalog implements
CatalogIf<Database> {
olapTable.readLock();
try {
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table' state is not NORMAL: " +
olapTable.getState());
- }
-
+ olapTable.checkNormalStateForAlter();
if (!truncateEntireTable) {
for (String partName :
tblRef.getPartitionNames().getPartitionNames()) {
Partition partition = olapTable.getPartition(partName);
@@ -2761,10 +2745,7 @@ public class InternalCatalog implements
CatalogIf<Database> {
olapTable = (OlapTable) db.getTableOrDdlException(copiedTbl.getId());
olapTable.writeLockOrDdlException();
try {
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table' state is not NORMAL: " +
olapTable.getState());
- }
-
+ olapTable.checkNormalStateForAlter();
// check partitions
for (Map.Entry<String, Long> entry : origPartitions.entrySet()) {
Partition partition = copiedTbl.getPartition(entry.getValue());
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
b/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
index eb6886855f..2815c9229e 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
@@ -412,6 +412,7 @@ public class FrontendServiceImpl implements
FrontendService.Iface {
olapTable.writeLockOrMetaException();
try {
+ olapTable.checkNormalStateForAlter();
List<ColumnDef> columnDefs = new ArrayList<ColumnDef>();
// prepare columnDefs
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/alter/MaterializedViewHandlerTest.java
b/fe/fe-core/src/test/java/org/apache/doris/alter/MaterializedViewHandlerTest.java
index ff517e3fb8..84bf21e8e5 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/alter/MaterializedViewHandlerTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/alter/MaterializedViewHandlerTest.java
@@ -299,8 +299,6 @@ public class MaterializedViewHandlerTest {
String mvName = "mv_1";
new Expectations() {
{
- olapTable.getState();
- result = OlapTable.OlapTableState.NORMAL;
olapTable.getName();
result = "table1";
olapTable.hasMaterializedIndex(mvName);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
