#6386 Enforce null check at Catalog.getDb and Database.getTable #6416

ccoffline · 2021-08-10T10:46:12Z

Proposed changes

fix #5378 #5391 #5688 #5973 #6155 and all replay NPE. All replay method can now throw MetaNotFoundException and caught to log a warning for potential inconsistent metadata cases.

try to establish a clear notice for future developer to check null.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)
Code refactor (Modify the code structure, format the code, etc...)
Optimization. Including functional usability improvements and performance improvements.
Dependency. Such as changes related to third-party components.
Other.

Checklist

I have created an issue on (Fix [Proposal] Enforce null check at Catalog.getDb and Database.getTable #6386) and described the bug/feature there in detail
Compiling and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
If these changes need document changes, I have updated the document
Any dependent changes have been merged

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

liutang123 · 2021-08-24T10:32:16Z

fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java

+                } finally {
+                    olapTable.readUnlock();
+                }
+            } catch (MetaNotFoundException ignored) {


This is equals to continue

liutang123 · 2021-08-24T12:37:28Z

fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java

-        } finally {
-            tbl.writeUnlock();
+        } catch (MetaNotFoundException e) {
+            LOG.warn("[INCONSISTENT META] changing table status failed after rollup job done", e);


This is not a INCONSISTENT META

This is called by replayAlterJobV2() and onJobDone(), which should both not throw this exception.

morningman · 2021-08-31T03:57:17Z

fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java

    }

-    public void replayModifyComment(ModifyCommentOperationLog operation) {
+    public void replayModifyComment(ModifyCommentOperationLog operation) throws MetaNotFoundException {


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.

It is considerable. But "replay" throwing any exception is an extremely big risk, which will cause all FE crush and cannot recover. These MetaNotFoundException are mostly thrown by getDb and getTable, due to the lock inconsistence that makes editlogs out of order. Semantically, throwing this exception means some metadata the editlog want to edit on is missing during replay, which makes this replay unable to continue. Just like getDb/Table, This kind of inconsistence cannot recover anyway, and these editlogs are theoretically only affect lost metadata.
I prefer to use MetaNotFoundException that indicate metadata has lost rather than other exception which may cause confuse. The inconsistence can be checked from warning log.

@ccoffline how about check db or table state after getting db or table lock , so that the edit log will always keep in order? we don't need some much check code?

@caiconghui we don't have any promise that the edit logs are in order. These check code is to prevent the worst from happening.
Edit logs that out of order may cause meta inconsistent, which has to be fixed sooner or later. We are exploring ways to ensure consistency and minimize the cost. Until then, we have to check all NPE or let all the FE to crash.

@ccoffline so we only need to simply check null for getdb and get table in replay operation and just simply skip the following operation? now, the only way that fe may crash is with that replay operation when fe restart?

is it better to split this PR into two PRs ? one for code refactor and another for db and table null check, I think it is easy to review and not-error-prone when PR is not so big? @ccoffline

morningman · 2021-08-31T04:07:25Z

fe/fe-core/src/main/java/org/apache/doris/alter/AlterHandler.java

+        // we log rollup job cancelled even if db is dropped.
+        // so check db != null here
+        Database db = catalog.getDbOrMetaException(alterJob.getDbId());
+        alterJob.replayCancel(db);


Not same logic as before.
If db is dropped, addFinishedOrCancelledAlterJob() will not be called.
better using try ... finally ?

I'll fix this.

morningman · 2021-08-31T04:19:34Z

fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java

+                    break;
+            }
+        } catch (MetaNotFoundException e) {
+            LOG.warn("[INCONSISTENT META] replay rollup job failed {}", replayedJob.getJobId(), e);


Why catch here? It is already been caught in Editlog's loadJournal() method?

It is called by replayAlterJobV2. I'm afraid it would be different from the origin.
https://github.com/apache/incubator-doris/blob/138e7e896dee6842d8af7583d59288190371cd86/fe/fe-core/src/main/java/org/apache/doris/alter/AlterHandler.java#L460-L469

morningman · 2021-08-31T04:22:34Z

fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java

+        try {
+            db = Catalog.getCurrentCatalog().getDbOrMetaException(dbId);
+        } catch (MetaNotFoundException e) {
+            throw new IOException("error happens when parsing create materialized view stmt: " + origStmt, e);


Can it continue?
If db does not exist, when this job is being executed, it will check it and cancel the job.

The origin code will throw NPE, I really don't know what to do here

OK, just keep it as before.

morningman · 2021-08-31T04:23:38Z

fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java

+        OlapTable olapTable;
        try {
-            olapTable = (OlapTable) db.getTableOrThrowException(tableName, Table.TableType.OLAP);
+            olapTable = db.getTableOrMetaException(tableName, Table.TableType.OLAP);


Just call getTableOrDdlException() ?

ok, that make sense

morningman · 2021-08-31T04:50:44Z

fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableRowCountAction.java

+                olapTable = db.getTableOrMetaException(tblName, Table.TableType.OLAP);
            } catch (MetaNotFoundException e) {
-                return ResponseEntityBuilder.okWithCommonError(e.getMessage());
+                return ResponseEntityBuilder.notFound(e.getMessage());


Use okWithCommonError.
404 is not for I can't find object in Doris meta, it is just for url not found in http protocol.

morningman · 2021-08-31T04:51:05Z

fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java

+                table = db.getTableOrMetaException(tblName, Table.TableType.OLAP);
            } catch (MetaNotFoundException e) {
-                return ResponseEntityBuilder.okWithCommonError(e.getMessage());
+                return ResponseEntityBuilder.notFound(e.getMessage());


Use okWithCommonError

morningman · 2021-08-31T04:51:14Z

fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java

+        try {
+            db = Catalog.getCurrentCatalog().getDbOrMetaException(fullDbName);
+        } catch (MetaNotFoundException e) {
+            return ResponseEntityBuilder.notFound(e.getMessage());


okWithCommonError

morningman · 2021-08-31T04:51:29Z

fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java

-        } finally {
-            db.readUnlock();
+        } catch (MetaNotFoundException e) {
+            return ResponseEntityBuilder.notFound(e.getMessage());


okWithCommonError

morningman · 2021-08-31T05:11:26Z

fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java

                }
            }
+        } catch (MetaNotFoundException e) {
+            LOG.warn("[INCONSISTENT META] replay failed {}", journal, e);


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.

I considered printing the original journal content, but gave up because of the complicated deserialization. JournalEntity has implemented toString() method and has already print the op code. It's better for all the Writable objects to implement toString() method.
I'll add e.getMessage() in one line and keep the exception stack trace.

morningman · 2021-09-01T14:25:48Z

fe/fe-core/src/main/java/org/apache/doris/analysis/LoadStmt.java

            }
-            if (dataDescription.getMergeType() != LoadTask.MergeType.APPEND
-                    && !((table instanceof OlapTable) && ((OlapTable) table).hasDeleteSign()) ) {
+            if (dataDescription.getMergeType() != LoadTask.MergeType.APPEND && table.hasDeleteSign()) {


Suggested change

if (dataDescription.getMergeType() != LoadTask.MergeType.APPEND && table.hasDeleteSign()) {

if (dataDescription.getMergeType() != LoadTask.MergeType.APPEND && !table.hasDeleteSign()) {

morningman

LGTM

morningman

LGTM

github-actions · 2021-09-03T05:32:56Z

PR approved by at least one committer and no changes requested.

github-actions · 2021-09-03T05:32:58Z

PR approved by anyone and no changes requested.

liutang123 reviewed Aug 24, 2021

View reviewed changes

morningman added area/catalog Issues or PRs related to catalog management kind/improvement labels Aug 31, 2021

morningman reviewed Aug 31, 2021

View reviewed changes

morningman reviewed Sep 1, 2021

View reviewed changes

#6386 Enforce null check at Catalog.getDb and Database.getTable

0ad7bba

morningman requested changes Sep 3, 2021

View reviewed changes

morningman approved these changes Sep 3, 2021

View reviewed changes

github-actions bot added approved Indicates a PR has been approved by one committer. reviewed labels Sep 3, 2021

morningman merged commit df54b34 into apache:master Sep 3, 2021

	if (dataDescription.getMergeType() != LoadTask.MergeType.APPEND && table.hasDeleteSign()) {
	if (dataDescription.getMergeType() != LoadTask.MergeType.APPEND && !table.hasDeleteSign()) {

#6386 Enforce null check at Catalog.getDb and Database.getTable #6416

#6386 Enforce null check at Catalog.getDb and Database.getTable #6416

Uh oh!

Conversation

ccoffline commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Checklist

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccoffline Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caiconghui Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morningman Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morningman left a comment

Choose a reason for hiding this comment

Uh oh!

morningman left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 3, 2021

ccoffline commented Aug 10, 2021 •

edited

Loading

ccoffline Aug 31, 2021 •

edited

Loading

caiconghui Aug 31, 2021 •

edited

Loading

morningman Aug 31, 2021 •

edited

Loading