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 2021/08/31 05:18:35 UTC
[GitHub] [incubator-doris] morningman commented on a change in pull request #6416: #6386 Enforce null check at Catalog.getDb and Database.getTable
morningman commented on a change in pull request #6416:
URL: https://github.com/apache/incubator-doris/pull/6416#discussion_r698963809
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -260,17 +251,11 @@ private void processModifyColumnComment(Database db, OlapTable tbl, List<AlterCl
}
}
- public void replayModifyComment(ModifyCommentOperationLog operation) {
+ public void replayModifyComment(ModifyCommentOperationLog operation) throws MetaNotFoundException {
Review comment:
Do we need a special Exception to handle those "replay" logic.
I am worried that `MetaNotFoundException` will be thrown elsewhere in the replay logic, but it may be unexpected.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java
##########
@@ -667,22 +638,26 @@ private void replayCancelled(RollupJobV2 replayedJob) {
@Override
public void replay(AlterJobV2 replayedJob) {
- RollupJobV2 replayedRollupJob = (RollupJobV2) replayedJob;
- switch (replayedJob.jobState) {
- case PENDING:
- replayCreateJob(replayedRollupJob);
- break;
- case WAITING_TXN:
- replayPendingJob(replayedRollupJob);
- break;
- case FINISHED:
- replayRunningJob(replayedRollupJob);
- break;
- case CANCELLED:
- replayCancelled(replayedRollupJob);
- break;
- default:
- break;
+ try {
+ RollupJobV2 replayedRollupJob = (RollupJobV2) replayedJob;
+ switch (replayedJob.jobState) {
+ case PENDING:
+ replayCreateJob(replayedRollupJob);
+ break;
+ case WAITING_TXN:
+ replayPendingJob(replayedRollupJob);
+ break;
+ case FINISHED:
+ replayRunningJob(replayedRollupJob);
+ break;
+ case CANCELLED:
+ replayCancelled(replayedRollupJob);
+ break;
+ default:
+ break;
+ }
+ } catch (MetaNotFoundException e) {
+ LOG.warn("[INCONSISTENT META] replay rollup job failed {}", replayedJob.getJobId(), e);
Review comment:
Why catch here? It is already been caught in Editlog's loadJournal() method?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java
##########
@@ -62,15 +62,12 @@ protected Object schema(
String fullDbName = getFullDbName(dbName);
// check privilege for select, otherwise return 401 HTTP status
checkTblAuth(ConnectContext.get().getCurrentUserIdentity(), fullDbName, tblName, PrivPredicate.SELECT);
- Database db = Catalog.getCurrentCatalog().getDb(fullDbName);
- if (db == null) {
- return ResponseEntityBuilder.okWithCommonError("Database [" + dbName + "] " + "does not exists");
- }
- Table table = null;
+ Table table;
try {
- table = db.getTableOrThrowException(tblName, Table.TableType.OLAP);
+ Database db = Catalog.getCurrentCatalog().getDbOrMetaException(fullDbName);
+ table = db.getTableOrMetaException(tblName, Table.TableType.OLAP);
} catch (MetaNotFoundException e) {
- return ResponseEntityBuilder.okWithCommonError(e.getMessage());
+ return ResponseEntityBuilder.notFound(e.getMessage());
Review comment:
Use `okWithCommonError`
##########
File path: fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableRowCountAction.java
##########
@@ -61,15 +61,12 @@ public Object count(
String fullDbName = getFullDbName(dbName);
// check privilege for select, otherwise return HTTP 401
checkTblAuth(ConnectContext.get().getCurrentUserIdentity(), fullDbName, tblName, PrivPredicate.SELECT);
- Database db = Catalog.getCurrentCatalog().getDb(fullDbName);
- if (db == null) {
- return ResponseEntityBuilder.okWithCommonError("Database [" + dbName + "] " + "does not exists");
- }
- OlapTable olapTable = null;
+ OlapTable olapTable;
try {
- olapTable = (OlapTable) db.getTableOrThrowException(tblName, Table.TableType.OLAP);
+ Database db = Catalog.getCurrentCatalog().getDbOrMetaException(fullDbName);
+ olapTable = db.getTableOrMetaException(tblName, Table.TableType.OLAP);
} catch (MetaNotFoundException e) {
- return ResponseEntityBuilder.okWithCommonError(e.getMessage());
+ return ResponseEntityBuilder.notFound(e.getMessage());
Review comment:
Use `okWithCommonError`.
404 is not for `I can't find object in Doris meta`, it is just for `url not found` in http protocol.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeJobV2.java
##########
@@ -218,9 +215,9 @@ protected void runPendingJob() throws AlterCancelException {
}
MarkedCountDownLatch<Long, Long> countDownLatch = new MarkedCountDownLatch<>(totalReplicaNum);
- OlapTable tbl = null;
+ OlapTable tbl;
try {
- tbl = (OlapTable) db.getTableOrThrowException(tableId, TableType.OLAP);
+ tbl = db.getTableOrMetaException(tableId, TableType.OLAP);
Review comment:
call `getTableOrException()` ?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -6341,12 +6264,12 @@ public long loadCluster(DataInputStream dis, long checksum) throws IOException,
// for adding BE to some Cluster, but loadCluster is after loadBackend.
cluster.setBackendIdList(latestBackendIds);
- String dbName = InfoSchemaDb.getFullInfoSchemaDbName(cluster.getName());
+ String dbName = InfoSchemaDb.getFullInfoSchemaDbName(cluster.getName());
InfoSchemaDb db;
// Use real Catalog instance to avoid InfoSchemaDb id continuously increment
// when checkpoint thread load image.
- if (Catalog.getServingCatalog().getFullNameToDb().containsKey(dbName)) {
- db = (InfoSchemaDb)Catalog.getServingCatalog().getFullNameToDb().get(dbName);
+ if (Catalog.getCurrentCatalog().getFullNameToDb().containsKey(dbName)) {
Review comment:
Must use `getServingCatalog()` here
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/AlterHandler.java
##########
@@ -302,40 +302,37 @@ protected void jobDone(AlterJob alterJob) {
}
}
- public void replayInitJob(AlterJob alterJob, Catalog catalog) {
- Database db = catalog.getDb(alterJob.getDbId());
+ public void replayInitJob(AlterJob alterJob, Catalog catalog) throws MetaNotFoundException {
+ Database db = catalog.getDbOrMetaException(alterJob.getDbId());
alterJob.replayInitJob(db);
// add rollup job
addAlterJob(alterJob);
}
- public void replayFinishing(AlterJob alterJob, Catalog catalog) {
- Database db = catalog.getDb(alterJob.getDbId());
+ public void replayFinishing(AlterJob alterJob, Catalog catalog) throws MetaNotFoundException {
+ Database db = catalog.getDbOrMetaException(alterJob.getDbId());
alterJob.replayFinishing(db);
alterJob.setState(JobState.FINISHING);
// !!! the alter job should add to the cache again, because the alter job is deserialized from journal
// it is a different object compared to the cache
addAlterJob(alterJob);
}
- public void replayFinish(AlterJob alterJob, Catalog catalog) {
- Database db = catalog.getDb(alterJob.getDbId());
+ public void replayFinish(AlterJob alterJob, Catalog catalog) throws MetaNotFoundException {
+ Database db = catalog.getDbOrMetaException(alterJob.getDbId());
alterJob.replayFinish(db);
alterJob.setState(JobState.FINISHED);
jobDone(alterJob);
}
- public void replayCancel(AlterJob alterJob, Catalog catalog) {
+ public void replayCancel(AlterJob alterJob, Catalog catalog) throws MetaNotFoundException {
removeAlterJob(alterJob.getTableId());
alterJob.setState(JobState.CANCELLED);
- Database db = catalog.getDb(alterJob.getDbId());
- if (db != null) {
- // we log rollup job cancelled even if db is dropped.
- // so check db != null here
- alterJob.replayCancel(db);
- }
-
+ // we log rollup job cancelled even if db is dropped.
+ // so check db != null here
+ Database db = catalog.getDbOrMetaException(alterJob.getDbId());
+ alterJob.replayCancel(db);
Review comment:
Not same logic as before.
If db is dropped, `addFinishedOrCancelledAlterJob()` will not be called.
better using `try ... finally` ?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java
##########
@@ -1048,11 +1049,17 @@ private boolean downloadAndDeserializeMetaInfo() {
}
private void replayCheckAndPrepareMeta() {
- Database db = catalog.getDb(dbId);
+ Database db;
+ try {
+ db = catalog.getDbOrMetaException(dbId);
Review comment:
I think we can add `throws MetaNotFoundException` to this method's signature.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -4548,72 +4476,43 @@ private void unprotectUpdateReplica(ReplicaPersistInfo info) {
replica.setBad(false);
}
- public void replayAddReplica(ReplicaPersistInfo info) {
- Database db = getDb(info.getDbId());
- OlapTable olapTable = (OlapTable) db.getTable(info.getTableId());
- if (olapTable == null) {
- /**
- * Same as replayUpdateReplica()
- */
- LOG.warn("Olap table is null when the add replica log is replayed, {}", info);
- return;
- }
+ public void replayAddReplica(ReplicaPersistInfo info) throws MetaNotFoundException {
+ Database db = this.getDbOrMetaException(info.getDbId());
+ OlapTable olapTable = db.getTableOrMetaException(info.getTableId(), TableType.OLAP);
olapTable.writeLock();
try {
- unprotectAddReplica(info);
+ unprotectAddReplica(olapTable, info);
} finally {
olapTable.writeUnlock();
}
}
- public void replayUpdateReplica(ReplicaPersistInfo info) {
- Database db = getDb(info.getDbId());
- OlapTable olapTable = (OlapTable) db.getTable(info.getTableId());
- if (olapTable == null) {
Review comment:
Better move this comment to where MetaNotFoundException is caught in `EditLog.java`. For developer to understand
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
##########
@@ -1889,17 +1889,14 @@ public void cancel(CancelStmt stmt) throws DdlException {
Preconditions.checkState(!Strings.isNullOrEmpty(dbName));
Preconditions.checkState(!Strings.isNullOrEmpty(tableName));
- Database db = Catalog.getCurrentCatalog().getDb(dbName);
- if (db == null) {
- throw new DdlException("Database[" + dbName + "] does not exist");
- }
+ Database db = Catalog.getCurrentCatalog().getDbOrDdlException(dbName);
AlterJob schemaChangeJob = null;
AlterJobV2 schemaChangeJobV2 = null;
- OlapTable olapTable = null;
+ OlapTable olapTable;
try {
- olapTable = (OlapTable) db.getTableOrThrowException(tableName, Table.TableType.OLAP);
+ olapTable = db.getTableOrMetaException(tableName, Table.TableType.OLAP);
Review comment:
Just call `getTableOrDdlException()` ?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java
##########
@@ -812,10 +787,14 @@ public void gsonPostProcess() throws IOException {
return;
}
// parse the define stmt to schema
- SqlParser parser = new SqlParser(new SqlScanner(new StringReader(origStmt.originStmt),
- SqlModeHelper.MODE_DEFAULT));
+ SqlParser parser = new SqlParser(new SqlScanner(new StringReader(origStmt.originStmt), SqlModeHelper.MODE_DEFAULT));
ConnectContext connectContext = new ConnectContext();
- Database db = Catalog.getCurrentCatalog().getDb(dbId);
+ Database db;
+ try {
+ db = Catalog.getCurrentCatalog().getDbOrMetaException(dbId);
+ } catch (MetaNotFoundException e) {
+ throw new IOException("error happens when parsing create materialized view stmt: " + origStmt, e);
Review comment:
Can it continue?
If db does not exist, when this job is being executed, it will check it and cancel the job.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java
##########
@@ -207,30 +210,25 @@ public Object getTableSchema(
String fullDbName = getFullDbName(dbName);
checkTblAuth(ConnectContext.get().getCurrentUserIdentity(), fullDbName, tblName, PrivPredicate.SHOW);
-
- Database db = Catalog.getCurrentCatalog().getDb(fullDbName);
- if (db == null) {
- return ResponseEntityBuilder.okWithCommonError("Database does not exist: " + fullDbName);
- }
-
String withMvPara = request.getParameter(PARAM_WITH_MV);
- boolean withMv = Strings.isNullOrEmpty(withMvPara) ? false : withMvPara.equals("1");
-
+ boolean withMv = !Strings.isNullOrEmpty(withMvPara) && withMvPara.equals("1");
- TableSchemaInfo tableSchemaInfo = new TableSchemaInfo();
- db.readLock();
try {
- Table tbl = db.getTable(tblName);
- if (tbl == null) {
- return ResponseEntityBuilder.okWithCommonError("Table does not exist: " + tblName);
+ Database db = Catalog.getCurrentCatalog().getDbOrMetaException(fullDbName);
+ db.readLock();
+ try {
+ Table tbl = db.getTableOrMetaException(tblName, Table.TableType.OLAP);
+
+ TableSchemaInfo tableSchemaInfo = new TableSchemaInfo();
+ tableSchemaInfo.setEngineType(tbl.getType().toString());
+ SchemaInfo schemaInfo = generateSchemaInfo(tbl, withMv);
+ tableSchemaInfo.setSchemaInfo(schemaInfo);
+ return ResponseEntityBuilder.ok(tableSchemaInfo);
+ } finally {
+ db.readUnlock();
}
-
- tableSchemaInfo.setEngineType(tbl.getType().toString());
- SchemaInfo schemaInfo = generateSchemaInfo(tbl, withMv);
- tableSchemaInfo.setSchemaInfo(schemaInfo);
- return ResponseEntityBuilder.ok(tableSchemaInfo);
- } finally {
- db.readUnlock();
+ } catch (MetaNotFoundException e) {
+ return ResponseEntityBuilder.notFound(e.getMessage());
Review comment:
okWithCommonError
##########
File path: fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java
##########
@@ -139,9 +140,11 @@ public Object getTables(
String fullDbName = getFullDbName(dbName);
- Database db = Catalog.getCurrentCatalog().getDb(fullDbName);
- if (db == null) {
- return ResponseEntityBuilder.okWithCommonError("Database does not exist: " + fullDbName);
+ Database db;
+ try {
+ db = Catalog.getCurrentCatalog().getDbOrMetaException(fullDbName);
+ } catch (MetaNotFoundException e) {
+ return ResponseEntityBuilder.notFound(e.getMessage());
Review comment:
okWithCommonError
##########
File path: fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java
##########
@@ -865,6 +862,8 @@ public static void loadJournal(Catalog catalog, JournalEntity journal) {
throw e;
}
}
+ } catch (MetaNotFoundException e) {
+ LOG.warn("[INCONSISTENT META] replay failed {}", journal, e);
Review comment:
add `e.getMessage()` so that it is easier to `grep` in one line.
And some of `journal` may not implement `toString()` method. So I think we can just print op code.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeJobV2.java
##########
@@ -370,14 +367,11 @@ protected void runWaitingTxnJob() throws AlterCancelException {
}
LOG.info("previous transactions are all finished, begin to send schema change tasks. job: {}", jobId);
- Database db = Catalog.getCurrentCatalog().getDb(dbId);
- if (db == null) {
- throw new AlterCancelException("Databasee " + dbId + " does not exist");
- }
+ Database db = Catalog.getCurrentCatalog().getDbOrException(dbId, s -> new AlterCancelException("Database " + s + " does not exist"));
- OlapTable tbl = null;
+ OlapTable tbl;
try {
- tbl = (OlapTable) db.getTableOrThrowException(tableId, TableType.OLAP);
+ tbl = db.getTableOrMetaException(tableId, TableType.OLAP);
Review comment:
call `getTableOrException()` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
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