morningman commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r530313863
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -128,30 +124,10 @@ public void
processDropMaterializedView(DropMaterializedViewStmt stmt) throws Dd
ErrorReport.reportDdlException(ErrorCode.ERR_BAD_DB_ERROR, dbName);
}
- db.writeLock();
- try {
- String tableName = stmt.getTableName().getTbl();
- Table table = db.getTable(tableName);
- // if table exists
- if (table == null) {
- ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR,
tableName);
- }
- // check table type
- if (table.getType() != TableType.OLAP) {
- throw new DdlException("Do not support non-OLAP table [" +
tableName + "] when drop materialized view");
- }
- // check table state
- OlapTable olapTable = (OlapTable) table;
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException("Table[" + table.getName() + "]'s state
is not NORMAL. "
- + "Do not allow doing DROP ops");
- }
- // drop materialized view
-
((MaterializedViewHandler)materializedViewHandler).processDropMaterializedView(stmt,
db, olapTable);
-
- } finally {
- db.writeUnlock();
- }
+ String tableName = stmt.getTableName().getTbl();
+ OlapTable olapTable = (OlapTable)
db.getTableOrThrowException(tableName, TableType.OLAP);
Review comment:
Missing `if (olapTable.getState() != OlapTableState.NORMAL) {` check?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/backup/BackupHandler.java
##########
@@ -274,25 +274,25 @@ private void backup(Repository repository, Database db,
BackupStmt stmt) throws
// This is just a pre-check to avoid most of invalid backup requests.
// Also calculate the signature for incremental backup check.
List<TableRef> tblRefs = stmt.getTableRefs();
- BackupMeta curBackupMeta = null;
- db.readLock();
- try {
- List<Table> backupTbls = Lists.newArrayList();
- for (TableRef tblRef : tblRefs) {
- String tblName = tblRef.getName().getTbl();
- Table tbl = db.getTable(tblName);
- if (tbl == null) {
-
ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR, tblName);
- }
- if (tbl.getType() != TableType.OLAP) {
-
ErrorReport.reportDdlException(ErrorCode.ERR_NOT_OLAP_TABLE, tblName);
- }
- OlapTable olapTbl = (OlapTable) tbl;
+ List<Table> backupTbls = Lists.newArrayList();
+ for (TableRef tblRef : tblRefs) {
+ String tblName = tblRef.getName().getTbl();
+ Table tbl = db.getTable(tblName);
Review comment:
Why not using `getOrThrowException`?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/load/Load.java
##########
@@ -3229,19 +3234,19 @@ public void unprotectDelete(DeleteInfo deleteInfo,
Database db) {
public void replayFinishAsyncDeleteJob(AsyncDeleteJob deleteJob, Catalog
catalog) {
Database db = catalog.getDb(deleteJob.getDbId());
- db.writeLock();
+ writeLock();
Review comment:
The lock order is wrong.
the origin lock order is `db lock` -> 'load lock'.
But here you use 'table lock' -> 'load lock'
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -232,54 +223,61 @@ private void processAlterExternalTable(AlterTableStmt
stmt, Table externalTable,
List<AlterClause> alterClauses = stmt.getOps();
AlterOperations currentAlterOps = new AlterOperations();
currentAlterOps.checkConflict(alterClauses);
-
if (currentAlterOps.hasRenameOp()) {
processRename(db, externalTable, alterClauses);
} else if (currentAlterOps.hasSchemaChangeOp()) {
- schemaChangeHandler.processExternalTable(alterClauses, db,
externalTable);
+ externalTable.writeLock();
+ try {
+ schemaChangeHandler.processExternalTable(alterClauses, db,
externalTable);
+ } finally {
+ externalTable.writeUnlock();
+ }
}
}
public void processAlterTable(AlterTableStmt stmt) throws UserException {
TableName dbTableName = stmt.getTbl();
String dbName = dbTableName.getDb();
+ String tableName = dbTableName.getTbl();
final String clusterName = stmt.getClusterName();
Database db = Catalog.getCurrentCatalog().getDb(dbName);
if (db == null) {
ErrorReport.reportDdlException(ErrorCode.ERR_BAD_DB_ERROR, dbName);
}
+ Table table = db.getTable(tableName);
+ if (table == null) {
+ ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR,
tableName);
+ }
List<AlterClause> alterClauses = Lists.newArrayList();
+ // some operations will take long time to process, need to be done
outside the table lock
+ boolean needProcessOutsideTableLock = false;
- // some operations will take long time to process, need to be done
outside the database lock
- boolean needProcessOutsideDatabaseLock = false;
- String tableName = dbTableName.getTbl();
- db.writeLock();
- try {
- Table table = db.getTable(tableName);
- if (table == null) {
- ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR,
tableName);
- }
+ // check conflict alter ops first
+ AlterOperations currentAlterOps = new AlterOperations();
+ currentAlterOps.checkConflict(alterClauses);
+ // check cluster capacity and db quota outside table lock to escape
dead lock, only need to check once.
+ if (currentAlterOps.needCheckCapacity()) {
+ Catalog.getCurrentSystemInfo().checkClusterCapacity(clusterName);
+ db.checkQuota();
+ }
- switch (table.getType()) {
- case OLAP:
- OlapTable olapTable = (OlapTable) table;
- needProcessOutsideDatabaseLock =
processAlterOlapTable(stmt, olapTable, alterClauses, clusterName, db);
- break;
- case ODBC:
- case MYSQL:
- case ELASTICSEARCH:
- processAlterExternalTable(stmt, table, db);
- return;
- default:
- throw new DdlException("Do not support alter " +
table.getType().toString() + " table[" + tableName + "]");
- }
- } finally {
- db.writeUnlock();
+ switch (table.getType()) {
Review comment:
I think we can just lock the table outside the `switch`, avoid call
lock/unlock everywhere inside the `processAlterOlapTable` and
`processAlterExternalTable`.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java
##########
@@ -664,18 +667,25 @@ private void checkAndPrepareMeta() {
remoteDataProperty, (short)
restoreReplicationNum,
remotePartitionInfo.getIsInMemory(remotePartId));
localTbl.addPartition(restoredPart);
+ } finally {
+ localTbl.writeUnlock();
}
- // add restored tables
- for (OlapTable tbl : restoredTbls) {
+ }
+
+ // add restored tables
+ for (OlapTable tbl : restoredTbls) {
+ db.writeLock();
+ try {
Review comment:
We can use `db.createTableWithLock()`
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -476,27 +464,48 @@ public void processAlterCluster(AlterSystemStmt stmt)
throws UserException {
private void processRename(Database db, OlapTable table, List<AlterClause>
alterClauses) throws DdlException {
for (AlterClause alterClause : alterClauses) {
if (alterClause instanceof TableRenameClause) {
- Catalog.getCurrentCatalog().renameTable(db, table,
(TableRenameClause) alterClause);
- break;
- } else if (alterClause instanceof RollupRenameClause) {
- Catalog.getCurrentCatalog().renameRollup(db, table,
(RollupRenameClause) alterClause);
- break;
- } else if (alterClause instanceof PartitionRenameClause) {
- Catalog.getCurrentCatalog().renamePartition(db, table,
(PartitionRenameClause) alterClause);
- break;
- } else if (alterClause instanceof ColumnRenameClause) {
- Catalog.getCurrentCatalog().renameColumn(db, table,
(ColumnRenameClause) alterClause);
+ db.writeLock();
+ table.writeLock();
+ try {
+ Catalog.getCurrentCatalog().renameTable(db, table,
(TableRenameClause) alterClause);
+ } finally {
+ table.writeUnlock();
+ db.writeUnlock();
+ }
break;
} else {
- Preconditions.checkState(false);
+ table.writeLock();
Review comment:
Put lock inside the method
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -476,27 +464,48 @@ public void processAlterCluster(AlterSystemStmt stmt)
throws UserException {
private void processRename(Database db, OlapTable table, List<AlterClause>
alterClauses) throws DdlException {
for (AlterClause alterClause : alterClauses) {
if (alterClause instanceof TableRenameClause) {
- Catalog.getCurrentCatalog().renameTable(db, table,
(TableRenameClause) alterClause);
- break;
- } else if (alterClause instanceof RollupRenameClause) {
- Catalog.getCurrentCatalog().renameRollup(db, table,
(RollupRenameClause) alterClause);
- break;
- } else if (alterClause instanceof PartitionRenameClause) {
- Catalog.getCurrentCatalog().renamePartition(db, table,
(PartitionRenameClause) alterClause);
- break;
- } else if (alterClause instanceof ColumnRenameClause) {
- Catalog.getCurrentCatalog().renameColumn(db, table,
(ColumnRenameClause) alterClause);
+ db.writeLock();
Review comment:
Better put these locks inside the `renameTable` method.
##########
File path:
fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
##########
@@ -722,14 +732,21 @@ public void processBatchDropRollup(List<AlterClause>
dropRollupClauses, Database
editLog.logBatchDropRollup(new BatchDropInfo(dbId, tableId,
indexIdSet));
LOG.info("finished drop rollup index[{}] in table[{}]",
String.join("", rollupNameSet), olapTable.getName());
} finally {
- db.writeUnlock();
+ olapTable.writeUnlock();
}
}
public void processDropMaterializedView(DropMaterializedViewStmt
dropMaterializedViewStmt, Database db,
OlapTable olapTable) throws DdlException, MetaNotFoundException {
- Preconditions.checkState(db.isWriteLockHeldByCurrentThread());
+ Preconditions.checkState(olapTable.isWriteLockHeldByCurrentThread());
+ olapTable.writeLock();
Review comment:
Why lock again?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -476,27 +464,48 @@ public void processAlterCluster(AlterSystemStmt stmt)
throws UserException {
private void processRename(Database db, OlapTable table, List<AlterClause>
alterClauses) throws DdlException {
for (AlterClause alterClause : alterClauses) {
if (alterClause instanceof TableRenameClause) {
- Catalog.getCurrentCatalog().renameTable(db, table,
(TableRenameClause) alterClause);
- break;
- } else if (alterClause instanceof RollupRenameClause) {
- Catalog.getCurrentCatalog().renameRollup(db, table,
(RollupRenameClause) alterClause);
- break;
- } else if (alterClause instanceof PartitionRenameClause) {
- Catalog.getCurrentCatalog().renamePartition(db, table,
(PartitionRenameClause) alterClause);
- break;
- } else if (alterClause instanceof ColumnRenameClause) {
- Catalog.getCurrentCatalog().renameColumn(db, table,
(ColumnRenameClause) alterClause);
+ db.writeLock();
+ table.writeLock();
+ try {
+ Catalog.getCurrentCatalog().renameTable(db, table,
(TableRenameClause) alterClause);
+ } finally {
+ table.writeUnlock();
+ db.writeUnlock();
+ }
break;
} else {
- Preconditions.checkState(false);
+ table.writeLock();
+ try {
+ if (alterClause instanceof RollupRenameClause) {
+ Catalog.getCurrentCatalog().renameRollup(db, table,
(RollupRenameClause) alterClause);
+ break;
+ } else if (alterClause instanceof PartitionRenameClause) {
+ Catalog.getCurrentCatalog().renamePartition(db, table,
(PartitionRenameClause) alterClause);
+ break;
+ } else if (alterClause instanceof ColumnRenameClause) {
+ Catalog.getCurrentCatalog().renameColumn(db, table,
(ColumnRenameClause) alterClause);
+ break;
+ } else {
+ Preconditions.checkState(false);
+ }
+ } finally {
+ table.writeUnlock();
+ }
}
}
}
private void processRename(Database db, Table table, List<AlterClause>
alterClauses) throws DdlException {
for (AlterClause alterClause : alterClauses) {
if (alterClause instanceof TableRenameClause) {
- Catalog.getCurrentCatalog().renameTable(db, table,
(TableRenameClause) alterClause);
+ db.writeLock();
Review comment:
Put lock inside the method
##########
File path: fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java
##########
@@ -1347,42 +1374,53 @@ public void cancelInternal(boolean isReplay) {
// clean restored objs
Database db = catalog.getDb(dbId);
if (db != null) {
- db.writeLock();
- try {
- // rollback table's state to NORMAL
- setTableStateToNormal(db);
+ // rollback table's state to NORMAL
+ setTableStateToNormal(db);
- // remove restored tbls
- for (OlapTable restoreTbl : restoredTbls) {
- LOG.info("remove restored table when cancelled: {}",
restoreTbl.getName());
+ // remove restored tbls
+ for (OlapTable restoreTbl : restoredTbls) {
+ LOG.info("remove restored table when cancelled: {}",
restoreTbl.getName());
+ restoreTbl.writeLock();
+ try {
for (Partition part : restoreTbl.getPartitions()) {
for (MaterializedIndex idx :
part.getMaterializedIndices(IndexExtState.VISIBLE)) {
for (Tablet tablet : idx.getTablets()) {
Catalog.getCurrentInvertedIndex().deleteTablet(tablet.getId());
}
}
}
+ } finally {
+ restoreTbl.writeUnlock();
+ }
+ db.writeLock();
+ try {
Review comment:
Use `db.dropTableWithLock()`
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -2043,12 +2044,7 @@ public long saveDb(DataOutputStream dos, long checksum)
throws IOException {
// Don't write information_schema db meta
if (!InfoSchemaDb.isInfoSchemaDb(dbName)) {
checksum ^= entry.getKey();
- db.readLock();
- try {
- db.write(dos);
- } finally {
- db.readUnlock();
- }
+ db.write(dos);
Review comment:
Is it safe to write db without lock?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -6673,86 +6680,75 @@ public void convertDistributionType(Database db,
OlapTable tbl) throws DdlExcept
editLog.logModifyDistributionType(tableInfo);
LOG.info("finished to modify distribution type of table: " +
tbl.getName());
} finally {
- db.writeUnlock();
+ tbl.writeUnlock();
}
}
public void replayConvertDistributionType(TableInfo tableInfo) {
Database db = getDb(tableInfo.getDbId());
- db.writeLock();
+ OlapTable tbl = (OlapTable) db.getTable(tableInfo.getTableId());
+ if (tbl == null) {
+ return;
+ }
+ tbl.writeLock();
try {
- OlapTable tbl = (OlapTable) db.getTable(tableInfo.getTableId());
tbl.convertRandomDistributionToHashDistribution();
LOG.info("replay modify distribution type of table: " +
tbl.getName());
} finally {
- db.writeUnlock();
+ tbl.writeUnlock();
}
}
/*
* The entry of replacing partitions with temp partitions.
*/
- public void replaceTempPartition(Database db, String tableName,
ReplacePartitionClause clause) throws DdlException {
+ public void replaceTempPartition(Database db, OlapTable olapTable,
ReplacePartitionClause clause) throws DdlException {
+ Preconditions.checkState(olapTable.isWriteLockHeldByCurrentThread());
List<String> partitionNames = clause.getPartitionNames();
List<String> tempPartitionNames = clause.getTempPartitionNames();
boolean isStrictRange = clause.isStrictRange();
boolean useTempPartitionName = clause.useTempPartitionName();
- db.writeLock();
- try {
- Table table = db.getTable(tableName);
- if (table == null) {
- ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR,
tableName);
- }
-
- if (table.getType() != TableType.OLAP) {
- throw new DdlException("Table[" + tableName + "] is not OLAP
table");
+ // check partition exist
+ for (String partName : partitionNames) {
+ if (!olapTable.checkPartitionNameExist(partName, false)) {
Review comment:
Does `checkPartitionNameExist ` need table lock?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -6673,86 +6680,75 @@ public void convertDistributionType(Database db,
OlapTable tbl) throws DdlExcept
editLog.logModifyDistributionType(tableInfo);
LOG.info("finished to modify distribution type of table: " +
tbl.getName());
} finally {
- db.writeUnlock();
+ tbl.writeUnlock();
}
}
public void replayConvertDistributionType(TableInfo tableInfo) {
Database db = getDb(tableInfo.getDbId());
- db.writeLock();
+ OlapTable tbl = (OlapTable) db.getTable(tableInfo.getTableId());
+ if (tbl == null) {
+ return;
+ }
+ tbl.writeLock();
try {
- OlapTable tbl = (OlapTable) db.getTable(tableInfo.getTableId());
tbl.convertRandomDistributionToHashDistribution();
LOG.info("replay modify distribution type of table: " +
tbl.getName());
} finally {
- db.writeUnlock();
+ tbl.writeUnlock();
}
}
/*
* The entry of replacing partitions with temp partitions.
*/
- public void replaceTempPartition(Database db, String tableName,
ReplacePartitionClause clause) throws DdlException {
+ public void replaceTempPartition(Database db, OlapTable olapTable,
ReplacePartitionClause clause) throws DdlException {
+ Preconditions.checkState(olapTable.isWriteLockHeldByCurrentThread());
List<String> partitionNames = clause.getPartitionNames();
List<String> tempPartitionNames = clause.getTempPartitionNames();
boolean isStrictRange = clause.isStrictRange();
boolean useTempPartitionName = clause.useTempPartitionName();
- db.writeLock();
- try {
- Table table = db.getTable(tableName);
- if (table == null) {
- ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR,
tableName);
- }
-
- if (table.getType() != TableType.OLAP) {
- throw new DdlException("Table[" + tableName + "] is not OLAP
table");
+ // check partition exist
+ for (String partName : partitionNames) {
+ if (!olapTable.checkPartitionNameExist(partName, false)) {
+ throw new DdlException("Partition[" + partName + "] does not
exist");
}
-
- OlapTable olapTable = (OlapTable) table;
- // check partition exist
- for (String partName : partitionNames) {
- if (!olapTable.checkPartitionNameExist(partName, false)) {
- throw new DdlException("Partition[" + partName + "] does
not exist");
- }
- }
- for (String partName : tempPartitionNames) {
- if (!olapTable.checkPartitionNameExist(partName, true)) {
- throw new DdlException("Temp partition[" + partName + "]
does not exist");
- }
+ }
+ for (String partName : tempPartitionNames) {
+ if (!olapTable.checkPartitionNameExist(partName, true)) {
+ throw new DdlException("Temp partition[" + partName + "] does
not exist");
}
-
- olapTable.replaceTempPartitions(partitionNames,
tempPartitionNames, isStrictRange, useTempPartitionName);
-
- // write log
- ReplacePartitionOperationLog info = new
ReplacePartitionOperationLog(db.getId(), olapTable.getId(),
- partitionNames, tempPartitionNames, isStrictRange,
useTempPartitionName);
- editLog.logReplaceTempPartition(info);
- LOG.info("finished to replace partitions {} with temp partitions
{} from table: {}",
- clause.getPartitionNames(),
clause.getTempPartitionNames(), tableName);
- } finally {
- db.writeUnlock();
}
+ olapTable.replaceTempPartitions(partitionNames, tempPartitionNames,
isStrictRange, useTempPartitionName);
Review comment:
Does `replaceTempPartitions` need table lock?
##########
File path:
fe/fe-core/src/main/java/org/apache/doris/clone/ColocateTableBalancer.java
##########
@@ -233,16 +232,16 @@ private void matchGroup() {
}
}
}
- } // end for tables
-
- // mark group as stable or unstable
- if (isGroupStable) {
- colocateIndex.markGroupStable(groupId, true);
- } else {
- colocateIndex.markGroupUnstable(groupId, true);
+ } finally {
+ olapTable.readUnlock();
}
- } finally {
- db.readUnlock();
+ } // end for tables
+
+ // mark group as stable or unstable
+ if (isGroupStable) {
+ colocateIndex.markGroupStable(groupId, true);
Review comment:
is it safe to `markGroupStable/markGroupUnstable` outside the lock?
##########
File path:
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/SparkLoadPendingTask.java
##########
@@ -133,13 +136,18 @@ private void createEtlJobConf() throws LoadException {
}
Map<Long, EtlTable> tables = Maps.newHashMap();
- db.readLock();
+ Map<Long, Set<Long>> tableIdToPartitionIds = Maps.newHashMap();
+ Set<Long> allPartitionsTableIds = Sets.newHashSet();
+ prepareTablePartitionInfos(db, tableIdToPartitionIds,
allPartitionsTableIds);
Review comment:
In origin implementation, this `prepareTablePartitionInfos` and
following logic is within same db lock.
But now you put them into 2 lock phases, which becomes not atomic.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java
##########
@@ -456,23 +437,23 @@ public void analyze(TQueryOptions tQueryOptions) throws
UserException {
if (parsedStmt instanceof QueryStmt
|| parsedStmt instanceof InsertStmt
|| parsedStmt instanceof CreateTableAsSelectStmt) {
- Map<String, Database> dbs = Maps.newTreeMap();
+ Map<Long, Table> tableMap = Maps.newTreeMap();
QueryStmt queryStmt;
Set<String> parentViewNameSet = Sets.newHashSet();
if (parsedStmt instanceof QueryStmt) {
queryStmt = (QueryStmt) parsedStmt;
- queryStmt.getDbs(analyzer, dbs, parentViewNameSet);
+ queryStmt.getTables(analyzer, tableMap, parentViewNameSet);
} else {
InsertStmt insertStmt;
if (parsedStmt instanceof InsertStmt) {
insertStmt = (InsertStmt) parsedStmt;
} else {
insertStmt = ((CreateTableAsSelectStmt)
parsedStmt).getInsertStmt();
}
- insertStmt.getDbs(analyzer, dbs, parentViewNameSet);
+ insertStmt.getTables(analyzer, tableMap, parentViewNameSet);
}
-
- lock(dbs);
+ List<Table> tables = Lists.newArrayList(tableMap.values());
Review comment:
Are you sure this can get table list in order?
I think it is more safe and clear to change `tableMap` to `tableList`, and
sort the `tableList` explicitly.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/master/ReportHandler.java
##########
@@ -14,6 +14,22 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
+// Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Why this license is different?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/load/Load.java
##########
@@ -398,7 +398,7 @@ private void addLoadJob(LoadJob job, Database db) throws
DdlException {
}
// check if table is in restore process
- db.readLock();
+ readLock();
Review comment:
I think we should hold the table's readLock before checking table' state
##########
File path: fe/fe-core/src/main/java/org/apache/doris/load/LoadChecker.java
##########
@@ -565,18 +568,14 @@ private void runOneQuorumFinishedDeleteJob(AsyncDeleteJob
job) {
load.removeDeleteJobAndSetState(job);
return;
}
- db.readLock();
- try {
- // if the delete job is quorum finished, just set it to finished
- job.clearTasks();
- job.setState(DeleteState.FINISHED);
- // log
- Catalog.getCurrentCatalog().getEditLog().logFinishAsyncDelete(job);
- load.removeDeleteJobAndSetState(job);
- LOG.info("delete job {} finished", job.getJobId());
- } finally {
- db.readUnlock();
- }
+
+ // if the delete job is quorum finished, just set it to finished
+ job.clearTasks();
Review comment:
This part should be protected by lock
##########
File path:
fe/fe-core/src/main/java/org/apache/doris/system/SystemInfoService.java
##########
@@ -894,18 +893,8 @@ public long getBackendReportVersion(long backendId) {
public void updateBackendReportVersion(long backendId, long
newReportVersion, long dbId) {
AtomicLong atomicLong = null;
if ((atomicLong = idToReportVersionRef.get(backendId)) != null) {
- Database db = Catalog.getCurrentCatalog().getDb(dbId);
- if (db != null) {
- db.readLock();
- try {
- atomicLong.set(newReportVersion);
- LOG.debug("update backend {} report version: {}, db: {}",
backendId, newReportVersion, dbId);
- } finally {
- db.readUnlock();
- }
- } else {
- LOG.warn("failed to update backend report version, db {} does
not exist", dbId);
- }
+ atomicLong.set(newReportVersion);
Review comment:
In original implementation, we use db locks to ensure mutual exclusion
and order.
So here we may still need to add table locks.
##########
File path:
fe/fe-core/src/main/java/org/apache/doris/task/HadoopLoadPendingTask.java
##########
@@ -70,41 +70,41 @@ public HadoopLoadPendingTask(LoadJob job) {
@Override
protected void createEtlRequest() throws Exception {
- db.readLock();
- try {
- EtlTaskConf taskConf = new EtlTaskConf();
- // output path
- taskConf.setOutputPath(getOutputPath());
- // output file pattern
- taskConf.setOutputFilePattern(job.getLabel() +
".%(table)s.%(view)s.%(bucket)s");
- // tables (partitions)
- Map<String, EtlPartitionConf> etlPartitions =
createEtlPartitions();
- Preconditions.checkNotNull(etlPartitions);
- taskConf.setEtlPartitions(etlPartitions);
+ EtlTaskConf taskConf = new EtlTaskConf();
Review comment:
Table's lock should be held to call method like `createEtlPartitions()`
##########
File path:
fe/fe-core/src/main/java/org/apache/doris/transaction/PublishVersionDaemon.java
##########
@@ -190,16 +190,18 @@ private void publishVersion() throws UserException {
LOG.warn("Database [{}] has been dropped.",
transactionState.getDbId());
continue;
}
- db.readLock();
- try {
- for (int i = 0; i <
transactionState.getTableIdList().size(); i++) {
- long tableId =
transactionState.getTableIdList().get(i);
- Table table = db.getTable(tableId);
- if (table == null || table.getType() !=
Table.TableType.OLAP) {
- LOG.warn("Table [{}] in database [{}] has
been dropped.", tableId, db.getFullName());
- continue;
- }
- OlapTable olapTable = (OlapTable) table;
+
+
+ for (int i = 0; i <
transactionState.getTableIdList().size(); i++) {
+ long tableId =
transactionState.getTableIdList().get(i);
+ Table table = db.getTable(tableId);
+ if (table == null || table.getType() !=
Table.TableType.OLAP) {
+ LOG.warn("Table [{}] in database [{}] has been
dropped.", tableId, db.getFullName());
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ olapTable.readLock();
Review comment:
Is it more safe to lock all tables at once outside the for loop?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/load/Load.java
##########
@@ -3229,19 +3234,19 @@ public void unprotectDelete(DeleteInfo deleteInfo,
Database db) {
public void replayFinishAsyncDeleteJob(AsyncDeleteJob deleteJob, Catalog
catalog) {
Database db = catalog.getDb(deleteJob.getDbId());
- db.writeLock();
+ writeLock();
Review comment:
All other place in Load.java should also be checked again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
