You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/06/04 12:23:07 UTC
[GitHub] [incubator-doris] caiconghui opened a new pull request #3775: Support read and write lock in table level to reduce lock competition
caiconghui opened a new pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775
This PR is to reduce lock competition by supporting read and write lock in table level. When we modify or read table's meta, we don't need to get db lock, just get table write or read lock. And when we get db lock, that means meta directly in db cannot be modified by other thread. Db lock only protect meta in Database class, while table lock protect meta in Table class.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
26 ColocateTableIndex.java
27 ColocateTableBalancer.java
**Meta**
28 Database.java
29 MetadataViewer.java
30 OlapTable.java
31 Table.java
32 InfoSchemaDb.java
33 MetaLockUtils.java
**MetaManager**
34 TabletStatMgr.java
35 DynamicPartitionScheduler.java
36 TabletChecker.java
37 TabletSchedCtx.java
38 TabletScheduler.java
**Proc**
39 EsPartitionsProcDir.java
40 EsShardProcDir.java
41 IndexInfoProcDir.java
42 IndicesProcDir.java
43 PartitionsProcDir.java
44 StatisticProcDir.java
45 TablesProcDir.java
46 TabletsProcDir.java
**Check**
47 CheckConsistencyJob.java
48 ConsistencyChecker.java
**Rest**
49 GetDdlStmtAction.java
50 MigrationAction.java
51 RowCountAction.java
52 StorageTypeCheckAction.java
53 TableQueryPlanAction.java
54 TableRowCountAction.java
55 TableSchemaAction.java
56 ShowDataAction.java
57 MetaService.java
**Load**
58 BrokerFileGroup.java
59 DeleteHandler.java
60 DeleteJob.java
61 ExportJob.java
62 Load.java
63 LoadChecker.java
64 BrokerLoadJob.java
65 LoadManager.java
66 KafkaRoutineLoadJob.java
67 RoutineLoadJob.java
68 BulkLoadJob.java
69 LoadJob.java
70 SparkLoadJob.java
**System**
71 ReportHandler.java
72 SystemInfoService.java
73 ConnectProcessor.java
**Task**
74 HadoopLoadPendingTask.java
75 LoadEtlTask.java
76 MiniLoadPendingTask.java
77 StreamLoadTask.java
78 SparkLoadPendingTask.java
**TransactionMgr**
79 DatabaseTransactionMgr.java
80 GlobalTransactionMgr.java
81 PublishVersionDaemon.java
**Rpc**
82 FrontendServiceImpl.java
83 MasterImpl.java
**Test**
84 SchemaChangeJobV2Test.java
85 RollupJobV2Test.java
86 DatabaseTest.java
87 AlterTest.java
88 DemoTest.java
89 AnotherDemoTest.java
90 MetaLockUtilsTets.java
91 InfoSchemaDbTest.java
92 TableTest.java
93 DeleteHandlerTest.java
94 StreamLoadPlannerTest.java
95 StreamLoadScanNodeTest.java
96 DatabaseTransactionMgrTest.java
97 GlobalTransactionMgrTest.java
98 StmtExecutorTest.java
99 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r535187757
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.common.util;
+
+import org.apache.doris.catalog.Database;
+import org.apache.doris.catalog.Table;
+
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+public class MetaLockUtils {
+
+ public static void readLockDatabases(List<Database> databaseList) {
+ for (Database database : databaseList) {
+ database.readLock();
+ }
+ }
+
+ public static void readUnlockDatabases(List<Database> databaseList) {
+ for (int i = databaseList.size() - 1; i >= 0; i--) {
+ databaseList.get(i).readUnlock();
+ }
+ }
+
+ public static void readLockTables(List<Table> tableList) {
+ for (Table table : tableList) {
+ table.readLock();
+ }
+ }
+
+ public static void readUnlockTables(List<Table> tableList) {
+ for (int i = tableList.size() - 1; i >= 0; i--) {
+ tableList.get(i).readUnlock();
+ }
+ }
+
+ public static void writeLockTables(List<Table> tableList) {
Review comment:
@wangbo
It should not happen, because we should always ensure that all meta sorted by id asc before lock them, otherwise would cause dead lock.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r535187757
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.common.util;
+
+import org.apache.doris.catalog.Database;
+import org.apache.doris.catalog.Table;
+
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+public class MetaLockUtils {
+
+ public static void readLockDatabases(List<Database> databaseList) {
+ for (Database database : databaseList) {
+ database.readLock();
+ }
+ }
+
+ public static void readUnlockDatabases(List<Database> databaseList) {
+ for (int i = databaseList.size() - 1; i >= 0; i--) {
+ databaseList.get(i).readUnlock();
+ }
+ }
+
+ public static void readLockTables(List<Table> tableList) {
+ for (Table table : tableList) {
+ table.readLock();
+ }
+ }
+
+ public static void readUnlockTables(List<Table> tableList) {
+ for (int i = tableList.size() - 1; i >= 0; i--) {
+ tableList.get(i).readUnlock();
+ }
+ }
+
+ public static void writeLockTables(List<Table> tableList) {
Review comment:
It should not happen, because we should always ensure that all meta sorted by id asc before lock them, otherwise would cause dead lock.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531430030
##########
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:
I think the final result is same if the table is dropped before loop or in loop, we just make sure that it is thread safe, and we has already add committed txn check if user want to drop table during loading data
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r435865138
##########
File path: fe/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -118,30 +118,30 @@ public void processDropMaterializedView(DropMaterializedViewStmt stmt) throws Dd
ErrorReport.reportDdlException(ErrorCode.ERR_BAD_DB_ERROR, dbName);
}
- db.writeLock();
+ String tableName = stmt.getTableName().getTbl();
+ Table table = db.getTable(tableName);
Review comment:
Here should throw userException or try and catch then throw ddl exception because we need some detailed error message?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r536082788
##########
File path: fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java
##########
@@ -265,17 +265,16 @@ private void executeDynamicPartition() {
String tableName;
boolean skipAddPartition = false;
OlapTable olapTable;
- db.readLock();
+ olapTable = (OlapTable) db.getTable(tableId);
Review comment:
because when drop table, we need to prevent that other thread drop table or create table too. but for get table, we just need to ensure that the get table operation is thread safe, and the final result is ok, we don't purse the strict consistency here for better performance.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java
##########
@@ -265,17 +265,16 @@ private void executeDynamicPartition() {
String tableName;
boolean skipAddPartition = false;
OlapTable olapTable;
- db.readLock();
+ olapTable = (OlapTable) db.getTable(tableId);
Review comment:
@wangbo
because when drop table, we need to prevent that other thread drop table or create table too. but for get table, we just need to ensure that the get table operation is thread safe, and the final result is ok, we don't purse the strict consistency here for better performance.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-638814025
ref #3743
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui merged pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui merged pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
It is hard to review the changed files at one time, so I classify the modified files into different groups. Please Pay more attention to Alter, Catalog, Load theme, which could be quite error prone.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
26 ColocateTableIndex.java
27 ColocateTableBalancer.java
**Meta**
28 Database.java
29 MetadataViewer.java
30 OlapTable.java
31 Table.java
32 InfoSchemaDb.java
33 MetaLockUtils.java
**MetaManager**
34 TabletStatMgr.java
35 DynamicPartitionScheduler.java
36 TabletChecker.java
37 TabletSchedCtx.java
38 TabletScheduler.java
**Proc**
39 EsPartitionsProcDir.java
40 EsShardProcDir.java
41 IndexInfoProcDir.java
42 IndicesProcDir.java
43 PartitionsProcDir.java
44 StatisticProcDir.java
45 TablesProcDir.java
46 TabletsProcDir.java
**Check**
47 CheckConsistencyJob.java
48 ConsistencyChecker.java
**Rest**
49 GetDdlStmtAction.java
50 MigrationAction.java
51 RowCountAction.java
52 StorageTypeCheckAction.java
53 TableQueryPlanAction.java
54 TableRowCountAction.java
55 TableSchemaAction.java
56 ShowDataAction.java
57 MetaService.java
**Load**
58 BrokerFileGroup.java
59 DeleteHandler.java
60 DeleteJob.java
61 ExportJob.java
62 Load.java
63 LoadChecker.java
64 BrokerLoadJob.java
65 LoadManager.java
66 KafkaRoutineLoadJob.java
67 RoutineLoadJob.java
68 BulkLoadJob.java
69 LoadJob.java
70 SparkLoadJob.java
**System**
71 ReportHandler.java
72 SystemInfoService.java
73 ConnectProcessor.java
**Task**
74 HadoopLoadPendingTask.java
75 LoadEtlTask.java
76 MiniLoadPendingTask.java
77 StreamLoadTask.java
78 SparkLoadPendingTask.java
**TransactionMgr**
79 DatabaseTransactionMgr.java
80 GlobalTransactionMgr.java
81 PublishVersionDaemon.java
**Rpc**
82 FrontendServiceImpl.java
83 MasterImpl.java
**Test**
84 SchemaChangeJobV2Test.java
85 RollupJobV2Test.java
86 DatabaseTest.java
87 AlterTest.java
88 DemoTest.java
89 AnotherDemoTest.java
90 MetaLockUtilsTets.java
91 InfoSchemaDbTest.java
92 TableTest.java
93 DeleteHandlerTest.java
94 StreamLoadPlannerTest.java
95 StreamLoadScanNodeTest.java
96 DatabaseTransactionMgrTest.java
97 GlobalTransactionMgrTest.java
98 StmtExecutorTest.java
99 SparkLoadJobTest.java
Looking forwards to lively discussions.
@kangkaisen @morningman @wangbo
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531485554
##########
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:
fix
##########
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:
done
##########
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:
done
##########
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:
done
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531423249
##########
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:
yes, tableMap is a treeMap which is used to for previous analysis, the treeMap will keep table id in In ascending order,
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
InfoSchemaDb
MetaLockUtils.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
ShowDataAction.java
StorageTypeCheckAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
InfoSchemaDb
MetaLockUtils.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r532149019
##########
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:
Never mind, the `AsyncDeleteJob` is deprecated, I will remove it later.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531426499
##########
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:
table's state is a volatile variable, so no need to hold table read lock
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531425224
##########
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:
I think it is thread safe, for markGroupStable and markGroupUnstable function
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531470228
##########
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:
if use getOrThrowException, should cast MetaNotFoundException to DdlException, or should change to much throw exception declaraction from Ddl exception to MetaNotFoundException
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531470831
##########
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:
use this function use cause side effect that edit log write create table info.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-724431718
Main Modification and Some Lock rule to discuss.
1. For meta data viewing, just simple use table read lock to replace database read lock
2. For meta reading, we don't pursue strong consistency, our goal is to ensure not dead lock and thread safe, such as db.getTable do not need db read lock
3.In order to escape dead lock, we keep db lock -> table lock -> other lock, for same meta level, we must sort meta by unique id before lock meta list.
4.Although db lock to table lock modification is large and difficult to review, but I think the heavy work is Meta modification, which in Alter, Load, Catalog theme, and logic in meta viewing is clear and less error prone.
5. When change property in one meta, we need to change meta lock, such as load, we only need lock table, but when we change property both in meta and its parent meta, such rename operation, we must lock both db and table
6. Finally. reduce unneeded lock, such as lock the same meta twice in one operation, reduce the lock scope to reduce the lock competition.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
It is hard to review the changed files at one time, so I classify the modified files into different groups. Please
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
26 ColocateTableIndex.java
27 ColocateTableBalancer.java
**Meta**
28 Database.java
29 MetadataViewer.java
30 OlapTable.java
31 Table.java
32 InfoSchemaDb.java
33 MetaLockUtils.java
**MetaManager**
34 TabletStatMgr.java
35 DynamicPartitionScheduler.java
36 TabletChecker.java
37 TabletSchedCtx.java
38 TabletScheduler.java
**Proc**
39 EsPartitionsProcDir.java
40 EsShardProcDir.java
41 IndexInfoProcDir.java
42 IndicesProcDir.java
43 PartitionsProcDir.java
44 StatisticProcDir.java
45 TablesProcDir.java
46 TabletsProcDir.java
**Check**
47 CheckConsistencyJob.java
48 ConsistencyChecker.java
**Rest**
49 GetDdlStmtAction.java
50 MigrationAction.java
51 RowCountAction.java
52 StorageTypeCheckAction.java
53 TableQueryPlanAction.java
54 TableRowCountAction.java
55 TableSchemaAction.java
56 ShowDataAction.java
57 MetaService.java
**Load**
58 BrokerFileGroup.java
59 DeleteHandler.java
60 DeleteJob.java
61 ExportJob.java
62 Load.java
63 LoadChecker.java
64 BrokerLoadJob.java
65 LoadManager.java
66 KafkaRoutineLoadJob.java
67 RoutineLoadJob.java
68 BulkLoadJob.java
69 LoadJob.java
70 SparkLoadJob.java
**System**
71 ReportHandler.java
72 SystemInfoService.java
73 ConnectProcessor.java
**Task**
74 HadoopLoadPendingTask.java
75 LoadEtlTask.java
76 MiniLoadPendingTask.java
77 StreamLoadTask.java
78 SparkLoadPendingTask.java
**TransactionMgr**
79 DatabaseTransactionMgr.java
80 GlobalTransactionMgr.java
81 PublishVersionDaemon.java
**Rpc**
82 FrontendServiceImpl.java
83 MasterImpl.java
**Test**
84 SchemaChangeJobV2Test.java
85 RollupJobV2Test.java
86 DatabaseTest.java
87 AlterTest.java
88 DemoTest.java
89 AnotherDemoTest.java
90 MetaLockUtilsTets.java
91 InfoSchemaDbTest.java
92 TableTest.java
93 DeleteHandlerTest.java
94 StreamLoadPlannerTest.java
95 StreamLoadScanNodeTest.java
96 DatabaseTransactionMgrTest.java
97 GlobalTransactionMgrTest.java
98 StmtExecutorTest.java
99 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
Rest
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
Load
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
**System**
63 ReportHandler.java
64 SystemInfoService.java
65 ConnectProcessor.java
**Task**
66 HadoopLoadPendingTask.java
67 LoadEtlTask.java
68 MiniLoadPendingTask.java
69 StreamLoadTask.java
**TransactionMgr**
70 DatabaseTransactionMgr.java
71 GlobalTransactionMgr.java
**Rpc**
72 FrontendServiceImpl.java
73 MasterImpl.java
**Test**
74 TableTest.java
75 DeleteHandlerTest.java
76 StreamLoadPlannerTest.java
77 StreamLoadScanNodeTest.java
78 DatabaseTransactionMgrTest.java
79 GlobalTransactionMgrTest.java
80 StmtExecutorTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
It is hard to review the changed files at one time, so I classify the modified files into different groups. Please Pay more attention to Alter, Catalog, Load theme, which could be quite error prone. And now I don't change httpv2 code, because it is not used now, so we can modify it later.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
26 ColocateTableIndex.java
27 ColocateTableBalancer.java
**Meta**
28 Database.java
29 MetadataViewer.java
30 OlapTable.java
31 Table.java
32 InfoSchemaDb.java
33 MetaLockUtils.java
**MetaManager**
34 TabletStatMgr.java
35 DynamicPartitionScheduler.java
36 TabletChecker.java
37 TabletSchedCtx.java
38 TabletScheduler.java
**Proc**
39 EsPartitionsProcDir.java
40 EsShardProcDir.java
41 IndexInfoProcDir.java
42 IndicesProcDir.java
43 PartitionsProcDir.java
44 StatisticProcDir.java
45 TablesProcDir.java
46 TabletsProcDir.java
**Check**
47 CheckConsistencyJob.java
48 ConsistencyChecker.java
**Rest**
49 GetDdlStmtAction.java
50 MigrationAction.java
51 RowCountAction.java
52 StorageTypeCheckAction.java
53 TableQueryPlanAction.java
54 TableRowCountAction.java
55 TableSchemaAction.java
56 ShowDataAction.java
57 MetaService.java
**Load**
58 BrokerFileGroup.java
59 DeleteHandler.java
60 DeleteJob.java
61 ExportJob.java
62 Load.java
63 LoadChecker.java
64 BrokerLoadJob.java
65 LoadManager.java
66 KafkaRoutineLoadJob.java
67 RoutineLoadJob.java
68 BulkLoadJob.java
69 LoadJob.java
70 SparkLoadJob.java
**System**
71 ReportHandler.java
72 SystemInfoService.java
73 ConnectProcessor.java
**Task**
74 HadoopLoadPendingTask.java
75 LoadEtlTask.java
76 MiniLoadPendingTask.java
77 StreamLoadTask.java
78 SparkLoadPendingTask.java
**TransactionMgr**
79 DatabaseTransactionMgr.java
80 GlobalTransactionMgr.java
81 PublishVersionDaemon.java
**Rpc**
82 FrontendServiceImpl.java
83 MasterImpl.java
**Test**
84 SchemaChangeJobV2Test.java
85 RollupJobV2Test.java
86 DatabaseTest.java
87 AlterTest.java
88 DemoTest.java
89 AnotherDemoTest.java
90 MetaLockUtilsTets.java
91 InfoSchemaDbTest.java
92 TableTest.java
93 DeleteHandlerTest.java
94 StreamLoadPlannerTest.java
95 StreamLoadScanNodeTest.java
96 DatabaseTransactionMgrTest.java
97 GlobalTransactionMgrTest.java
98 StmtExecutorTest.java
99 SparkLoadJobTest.java
Looking forwards to lively discussions.
@kangkaisen @morningman @wangbo
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
InfoSchemaDb
MetaLockUtils.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
ShowDataAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 StmtExecutor.java
15 ShowExecutor.java
**Backup**
16 BackupHandler.java
17 BackupJob.java
18 RestoreJob.java
**Catalog**
19 Catalog.java
20 CatalogRecycleBin.java
**Colocate**
21 ColocateTableIndex.java
22 ColocateTableBalancer.java
**Meta**
23 Database.java
24 MetadataViewer.java
25 OlapTable.java
26 Table.java
**MetaManager**
27 TabletStatMgr.java
28 DynamicPartitionScheduler.java
29 TabletChecker.java
30 TabletSchedCtx.java
31 TabletScheduler.java
**Proc**
32 EsPartitionsProcDir.java
33 EsShardProcDir.java
34 IndexInfoProcDir.java
35 IndicesProcDir.java
36 PartitionsProcDir.java
37 StatisticProcDir.java
38 TablesProcDir.java
39 TabletsProcDir.java
**Check**
40 CheckConsistencyJob.java
41 ConsistencyChecker.java
Rest
42 GetDdlStmtAction.java
43 MigrationAction.java
44 RowCountAction.java
45 StorageTypeCheckAction.java
46 TableQueryPlanAction.java
47 TableRowCountAction.java
48 TableSchemaAction.java
Load
49 BrokerFileGroup.java
50 DeleteHandler.java
51 DeleteJob.java
52 ExportJob.java
53 Load.java
54 LoadChecker.java
55 BrokerLoadJob.java
56 LoadManager.java
57 KafkaRoutineLoadJob.java
58 RoutineLoadJob.java
**System**
59 ReportHandler.java
60 SystemInfoService.java
61 ConnectProcessor.java
**Task**
62 HadoopLoadPendingTask.java
63 LoadEtlTask.java
64 MiniLoadPendingTask.java
65 StreamLoadTask.java
**TransactionMgr**
66 DatabaseTransactionMgr.java
67 GlobalTransactionMgr.java
**Rpc**
68 FrontendServiceImpl.java
69 MasterImpl.java
**Test**
70 TableTest.java
71 DeleteHandlerTest.java
72 StreamLoadPlannerTest.java
73 StreamLoadScanNodeTest.java
74 DatabaseTransactionMgrTest.java
75 GlobalTransactionMgrTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-724431718
Main Modification and Some Lock rule to discuss.
1. For meta data viewing, just simple use table read lock to replace database read lock.
2. For meta reading, we don't pursue strong consistency, our goal is to ensure not dead lock and thread safe, such as db.getTable do not need db read lock.
3. In order to escape dead lock, we keep db lock -> table lock -> other lock, for same meta level, we must sort meta by unique id before lock meta list.
4. Although db lock to table lock modification is large and difficult to review, but I think the heavy work is Meta modification, which in Alter, Load, Catalog theme, and logic in meta viewing is clear and less error prone.
5. When change property in one meta, we need to change meta lock, such as load, we only need lock table, but when we change property both in meta and its parent meta, such rename operation, we must lock both db and table
6. Finally. reduce unneeded lock, such as lock the same meta twice in one operation, reduce the lock scope to reduce the lock competition.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r435864322
##########
File path: fe/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
##########
@@ -1015,13 +1017,8 @@ private void runOldAlterJob() {
continue;
}
- db.writeLock();
- try {
- OlapTable olapTable = (OlapTable) db.getTable(rollupJob.getTableId());
- rollupJob.cancel(olapTable, "cancelled");
- } finally {
- db.writeUnlock();
- }
+ OlapTable olapTable = (OlapTable) db.getTable(rollupJob.getTableId());
Review comment:
Why don't add the table lock？
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531420212
##########
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:
for Catalog dumpImage or saveImage will call this function. for the first, lock all dbs already. for second, only invoke by checkpoint thread, so it is thread safe
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
**System**
63 ReportHandler.java
64 SystemInfoService.java
65 ConnectProcessor.java
**Task**
66 HadoopLoadPendingTask.java
67 LoadEtlTask.java
68 MiniLoadPendingTask.java
69 StreamLoadTask.java
**TransactionMgr**
70 DatabaseTransactionMgr.java
71 GlobalTransactionMgr.java
**Rpc**
72 FrontendServiceImpl.java
73 MasterImpl.java
**Test**
74 TableTest.java
75 DeleteHandlerTest.java
76 StreamLoadPlannerTest.java
77 StreamLoadScanNodeTest.java
78 DatabaseTransactionMgrTest.java
79 GlobalTransactionMgrTest.java
80 StmtExecutorTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531491646
##########
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:
I cannot find the suitable lock to protect this part
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
It is hard to review the changed files at one time, so I classify the modified files into different groups. Please Pay more attention to Alter, Catalog, Load theme, which could be quite error prone.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
26 ColocateTableIndex.java
27 ColocateTableBalancer.java
**Meta**
28 Database.java
29 MetadataViewer.java
30 OlapTable.java
31 Table.java
32 InfoSchemaDb.java
33 MetaLockUtils.java
**MetaManager**
34 TabletStatMgr.java
35 DynamicPartitionScheduler.java
36 TabletChecker.java
37 TabletSchedCtx.java
38 TabletScheduler.java
**Proc**
39 EsPartitionsProcDir.java
40 EsShardProcDir.java
41 IndexInfoProcDir.java
42 IndicesProcDir.java
43 PartitionsProcDir.java
44 StatisticProcDir.java
45 TablesProcDir.java
46 TabletsProcDir.java
**Check**
47 CheckConsistencyJob.java
48 ConsistencyChecker.java
**Rest**
49 GetDdlStmtAction.java
50 MigrationAction.java
51 RowCountAction.java
52 StorageTypeCheckAction.java
53 TableQueryPlanAction.java
54 TableRowCountAction.java
55 TableSchemaAction.java
56 ShowDataAction.java
57 MetaService.java
**Load**
58 BrokerFileGroup.java
59 DeleteHandler.java
60 DeleteJob.java
61 ExportJob.java
62 Load.java
63 LoadChecker.java
64 BrokerLoadJob.java
65 LoadManager.java
66 KafkaRoutineLoadJob.java
67 RoutineLoadJob.java
68 BulkLoadJob.java
69 LoadJob.java
70 SparkLoadJob.java
**System**
71 ReportHandler.java
72 SystemInfoService.java
73 ConnectProcessor.java
**Task**
74 HadoopLoadPendingTask.java
75 LoadEtlTask.java
76 MiniLoadPendingTask.java
77 StreamLoadTask.java
78 SparkLoadPendingTask.java
**TransactionMgr**
79 DatabaseTransactionMgr.java
80 GlobalTransactionMgr.java
81 PublishVersionDaemon.java
**Rpc**
82 FrontendServiceImpl.java
83 MasterImpl.java
**Test**
84 SchemaChangeJobV2Test.java
85 RollupJobV2Test.java
86 DatabaseTest.java
87 AlterTest.java
88 DemoTest.java
89 AnotherDemoTest.java
90 MetaLockUtilsTets.java
91 InfoSchemaDbTest.java
92 TableTest.java
93 DeleteHandlerTest.java
94 StreamLoadPlannerTest.java
95 StreamLoadScanNodeTest.java
96 DatabaseTransactionMgrTest.java
97 GlobalTransactionMgrTest.java
98 StmtExecutorTest.java
99 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
InfoSchemaDb
MetaLockUtils.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
ShowDataAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
SparkLoadPendingTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
PublishVersionDaemon.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
SchemaChangeJobV2Test.java
RollupJobV2Test.java
DatabaseTest.java
AlterTest.java
DemoTest.java
AnotherDemoTest.java
MetaLockUtilsTets.java
InfoSchemaDbTest.java
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] wangbo commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
wangbo commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r536109020
##########
File path: fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java
##########
@@ -265,17 +265,16 @@ private void executeDynamicPartition() {
String tableName;
boolean skipAddPartition = false;
OlapTable olapTable;
- db.readLock();
+ olapTable = (OlapTable) db.getTable(tableId);
Review comment:
I get, you mean ```Performance is ahead of NPE```.
For the second point about ```lock sequence ```.
I think it should become programming specifications for Doris.
```
1 If try to lock multiple tables or dbs, it should use getXXXInOrderMethod.
2 If try to lock different type locks, lock sequence should be guaranteed.
3 The appearance of nested locks of the same type should be avoided.
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
26 ColocateTableIndex.java
27 ColocateTableBalancer.java
**Meta**
28 Database.java
29 MetadataViewer.java
30 OlapTable.java
31 Table.java
32 InfoSchemaDb.java
33 MetaLockUtils.java
**MetaManager**
34 TabletStatMgr.java
35 DynamicPartitionScheduler.java
36 TabletChecker.java
37 TabletSchedCtx.java
38 TabletScheduler.java
**Proc**
39 EsPartitionsProcDir.java
40 EsShardProcDir.java
41 IndexInfoProcDir.java
42 IndicesProcDir.java
43 PartitionsProcDir.java
44 StatisticProcDir.java
45 TablesProcDir.java
46 TabletsProcDir.java
**Check**
47 CheckConsistencyJob.java
48 ConsistencyChecker.java
**Rest**
49 GetDdlStmtAction.java
50 MigrationAction.java
51 RowCountAction.java
52 StorageTypeCheckAction.java
53 TableQueryPlanAction.java
54 TableRowCountAction.java
55 TableSchemaAction.java
56 ShowDataAction.java
57 MetaService.java
**Load**
58 BrokerFileGroup.java
59 DeleteHandler.java
60 DeleteJob.java
61 ExportJob.java
62 Load.java
63 LoadChecker.java
64 BrokerLoadJob.java
65 LoadManager.java
66 KafkaRoutineLoadJob.java
67 RoutineLoadJob.java
68 BulkLoadJob.java
69 LoadJob.java
70 SparkLoadJob.java
**System**
71 ReportHandler.java
72 SystemInfoService.java
73 ConnectProcessor.java
**Task**
74 HadoopLoadPendingTask.java
75 LoadEtlTask.java
76 MiniLoadPendingTask.java
77 StreamLoadTask.java
78 SparkLoadPendingTask.java
**TransactionMgr**
79 DatabaseTransactionMgr.java
80 GlobalTransactionMgr.java
81 PublishVersionDaemon.java
**Rpc**
82 FrontendServiceImpl.java
83 MasterImpl.java
**Test**
84 SchemaChangeJobV2Test.java
85 RollupJobV2Test.java
86 DatabaseTest.java
87 AlterTest.java
88 DemoTest.java
89 AnotherDemoTest.java
90 MetaLockUtilsTets.java
91 InfoSchemaDbTest.java
92 TableTest.java
93 DeleteHandlerTest.java
94 StreamLoadPlannerTest.java
95 StreamLoadScanNodeTest.java
96 DatabaseTransactionMgrTest.java
97 GlobalTransactionMgrTest.java
98 StmtExecutorTest.java
99 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531466818
##########
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:
atomicLong is AtomicLong,so I think it is meanless for add table lock or db lock?
##########
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:
add table lock in createEtlPartitions
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531421620
##########
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:
already get table lock outside replaceTempPartition
##########
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:
already get table lock outside replaceTempPartition
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r536084860
##########
File path: fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java
##########
@@ -265,17 +265,16 @@ private void executeDynamicPartition() {
String tableName;
boolean skipAddPartition = false;
OlapTable olapTable;
- db.readLock();
+ olapTable = (OlapTable) db.getTable(tableId);
Review comment:
Another consideration is to avoid deadlock， all lock sequence is db lock -> table lock -> other lock, and if we has get table lock and sometimes need to get table again from db, there may cause dead lock, so db get table operation not get db read lock anymore
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531469161
##########
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:
fix
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-639550026
> Hi @caiconghui , first of all, this change is worth it.
>
> But I don't think this is a good way to achieve it. In the previous implementation, it was not a good design to expose the database lock outside, and there have been multiple deadlocks caused by lock nesting problems.
>
> When the lock granularity is refined to the table level, this way of exposing locks will bring more error-proneness. And so many changes to locks in one PR are difficult to review.
>
> I suggest to refer to the implementation of kudu [MetadataGroupLock](https://github.com/apache/kudu/blob/master/src/kudu/master/catalog_manager.h) to abstract the operation of the lock and then gradually modify the relevant code.
+1.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-639547289
I just reviewed a little, and submitted review accidentally.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531514359
##########
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:
done
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
InfoSchemaDb
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531487397
##########
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:
some function need db lock and table lock but some function only need table lock, what't more, If the nesting function is too deep， it is easy to repeat locking， if we lock small scope for modificaion， which will be more clear although we may write many lock code
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r532149313
##########
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:
OK, LGTM
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] wangbo commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
wangbo commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r536067102
##########
File path: fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java
##########
@@ -265,17 +265,16 @@ private void executeDynamicPartition() {
String tableName;
boolean skipAddPartition = false;
OlapTable olapTable;
- db.readLock();
+ olapTable = (OlapTable) db.getTable(tableId);
Review comment:
When drop a table from database, a db lock will be held;
Why not get a db read lock here when get a table from database.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-638814025
ref #3775
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-724431718
Main Modification and Some Lock rule to discuss.
1. For meta data viewing, just simple use table read lock to replace database read lock
2. For meta reading, we don't pursue strong consistency, our goal is to ensure not dead lock and thread safe, such as db.getTable
3.In order to escape dead lock, we keep db lock -> table lock -> other lock, for same meta level, we must sort meta by unique id before lock meta.
4.Although db lock to table lock modification is large and difficult to review, but I think the heavy work is Meta modification, which in Alter, Load, Catalog theme, and logic in meta viewing is clear and less error prone.
6. When change property in one meta, we need to change meta lock, such as load, we only need lock table, but when we change property both in meta and its parent meta, such rename operation, we must lock both db and table
7. Finally. reduce unneeded lock, such as lock the same meta twice in one operation, reduce the lock scope to reduce the lock competition.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r435863730
##########
File path: fe/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -169,18 +169,19 @@ public void processAlterTable(AlterTableStmt stmt) throws UserException {
// some operations will take long time to process, need to be done outside the databse 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);
- }
- if (table.getType() != TableType.OLAP) {
- throw new DdlException("Do not support alter non-OLAP table[" + tableName + "]");
- }
- OlapTable olapTable = (OlapTable) table;
+ Table table = db.getTable(tableName);
Review comment:
Could use `getTableOrThrowException`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531472210
##########
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:
It is an incorrect modification.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r435861404
##########
File path: fe/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -118,30 +118,30 @@ public void processDropMaterializedView(DropMaterializedViewStmt stmt) throws Dd
ErrorReport.reportDdlException(ErrorCode.ERR_BAD_DB_ERROR, dbName);
}
- db.writeLock();
+ String tableName = stmt.getTableName().getTbl();
+ Table table = db.getTable(tableName);
Review comment:
Could use `getTableOrThrowException `
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui edited a comment on pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui edited a comment on pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#issuecomment-644728422
Lock rule :
db lock -> table lock -> other lock or synchronized function { db lock -> table lock}, not allowed to get same lock more than one time when already hold lock , in order to escape potential dead lock.
It is hard to review the changed files at one time, so I classify the modified files into different groups.
**Alter**
1 Alter.java
2 AlterHandler.java
3 AlterJobV2.java
4 MaterializedViewHandler.java
5 RollupJob.java
6 RollupJobV2.java
7 SchemaChangeHandler.java
8 SchemaChangeJob.java
9 SchemaChangeJobV2.java
**Stmt**
10 DescribeStmt.java
11 ExportStmt.java
12 ShowDataStmt.java
13 ShowPartitionsStmt.java
14 InsertStmt.java
15 QueryStmt.java
16 SelectStmt.java
17 SetOperationStmt.java
18 WithClause.java
19 StmtExecutor.java
20 ShowExecutor.java
**Backup**
21 BackupHandler.java
22 BackupJob.java
23 RestoreJob.java
**Catalog**
24 Catalog.java
25 CatalogRecycleBin.java
**Colocate**
25 ColocateTableIndex.java
26 ColocateTableBalancer.java
**Meta**
27 Database.java
28 MetadataViewer.java
29 OlapTable.java
30 Table.java
InfoSchemaDb
MetaLockUtils.java
**MetaManager**
31 TabletStatMgr.java
32 DynamicPartitionScheduler.java
33 TabletChecker.java
34 TabletSchedCtx.java
35 TabletScheduler.java
**Proc**
36 EsPartitionsProcDir.java
37 EsShardProcDir.java
38 IndexInfoProcDir.java
39 IndicesProcDir.java
40 PartitionsProcDir.java
41 StatisticProcDir.java
42 TablesProcDir.java
43 TabletsProcDir.java
**Check**
44 CheckConsistencyJob.java
45 ConsistencyChecker.java
**Rest**
46 GetDdlStmtAction.java
47 MigrationAction.java
48 RowCountAction.java
49 StorageTypeCheckAction.java
50 TableQueryPlanAction.java
51 TableRowCountAction.java
52 TableSchemaAction.java
ShowDataAction.java
**Load**
53 BrokerFileGroup.java
54 DeleteHandler.java
55 DeleteJob.java
56 ExportJob.java
57 Load.java
58 LoadChecker.java
59 BrokerLoadJob.java
60 LoadManager.java
61 KafkaRoutineLoadJob.java
62 RoutineLoadJob.java
63 BulkLoadJob.java
64 LoadJob.java
65 SparkLoadJob.java
SparkLoadPendingTask.java
**System**
66 ReportHandler.java
67 SystemInfoService.java
68 ConnectProcessor.java
**Task**
69 HadoopLoadPendingTask.java
70 LoadEtlTask.java
71 MiniLoadPendingTask.java
72 StreamLoadTask.java
**TransactionMgr**
73 DatabaseTransactionMgr.java
74 GlobalTransactionMgr.java
**Rpc**
75 FrontendServiceImpl.java
76 MasterImpl.java
**Test**
77 TableTest.java
78 DeleteHandlerTest.java
79 StreamLoadPlannerTest.java
80 StreamLoadScanNodeTest.java
81 DatabaseTransactionMgrTest.java
82 GlobalTransactionMgrTest.java
83 StmtExecutorTest.java
84 SparkLoadJobTest.java
Looking forwards to lively discussions.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r435871389
##########
File path: fe/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java
##########
@@ -1015,13 +1017,8 @@ private void runOldAlterJob() {
continue;
}
- db.writeLock();
- try {
- OlapTable olapTable = (OlapTable) db.getTable(rollupJob.getTableId());
- rollupJob.cancel(olapTable, "cancelled");
- } finally {
- db.writeUnlock();
- }
+ OlapTable olapTable = (OlapTable) db.getTable(rollupJob.getTableId());
Review comment:
because olap table could be null, so I add write lock in the cancel function
if (olapTable != null) {
olapTable.writeLock();
try {
Preconditions.checkState(olapTable.getId() == tableId);
for (Partition partition : olapTable.getPartitions()) {
if (partition.getState() == PartitionState.ROLLUP) {
partition.setState(PartitionState.NORMAL);
}
}
if (olapTable.getState() == OlapTableState.ROLLUP) {
olapTable.setState(OlapTableState.NORMAL);
}
} finally {
olapTable.writeUnlock();
}
}
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] wangbo commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
wangbo commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r535123440
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.common.util;
+
+import org.apache.doris.catalog.Database;
+import org.apache.doris.catalog.Table;
+
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+public class MetaLockUtils {
+
+ public static void readLockDatabases(List<Database> databaseList) {
+ for (Database database : databaseList) {
+ database.readLock();
+ }
+ }
+
+ public static void readUnlockDatabases(List<Database> databaseList) {
+ for (int i = databaseList.size() - 1; i >= 0; i--) {
+ databaseList.get(i).readUnlock();
+ }
+ }
+
+ public static void readLockTables(List<Table> tableList) {
+ for (Table table : tableList) {
+ table.readLock();
+ }
+ }
+
+ public static void readUnlockTables(List<Table> tableList) {
+ for (int i = tableList.size() - 1; i >= 0; i--) {
+ tableList.get(i).readUnlock();
+ }
+ }
+
+ public static void writeLockTables(List<Table> tableList) {
Review comment:
I find a corner case as below:
There are three threads wants to get table lock:
T1(thread 1) try lock(TableA,TableB)
T2(thread 2) try lock(TableB,TableC)
T3(thread 3) try lock(TableC,TableA)
If T1 get TableA's lock and T2 get TableB's lock and T3 get TableC's lock at the same time ,
Then three threads try get next table's lock ,dead lock happends.
T1 try lock TableB, but TableB is held by T2.
T2 try lock TableC,but TableC is held by T3.
T3 try lock TableA,but TableA is held by T1.
Can the current implementation solve this case?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531485398
##########
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:
I think it doesn't matter? if i modify table meta or drop table after db read lock, the final result is still same?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] caiconghui commented on a change in pull request #3775: Support read and write lock in table level to reduce lock competition
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #3775:
URL: https://github.com/apache/incubator-doris/pull/3775#discussion_r531468856
##########
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:
processDropMaterializedView has check table state operation
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org