Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BugFix] Fix the issue where FE restart fails when creating a table containing too many tablets #53062

Merged
merged 1 commit into from
Nov 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions fe/fe-core/src/main/java/com/starrocks/catalog/Database.java
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,17 @@ public boolean registerTableUnlocked(Table table) {
return true;
}

public void unRegisterTableUnlocked(Table table) {
if (table == null) {
return;
}

idToTable.remove(table.getId());
if (!table.isTemporaryTable()) {
nameToTable.remove(table.getName());
}
}

public void dropTable(String tableName, boolean isSetIfExists, boolean isForce) throws DdlException {
Table table;
Locker locker = new Locker();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
The unRegisterTableUnlocked method treats temporary and non-temporary tables the same, which may lead to removal inconsistencies across different collections if there's additional logic intended for these types.

You can modify the code like this:

public void unRegisterTableUnlocked(Table table) {
    if (table == null) {
        return;
    }

    idToTable.remove(table.getId());
    
    if (!table.isTemporaryTable()) {
        nameToTable.remove(table.getName());
    }
}

This modification ensures that both temporary and non-temporary tables are correctly removed from the idToTable, but only non-temporary tables are removed from the nameToTable, assuming the original intent was to handle them differently.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2021-present StarRocks, Inc. All rights reserved.
//
// Licensed 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
//
// https://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 com.starrocks.journal;

public class SerializeException extends RuntimeException {
public SerializeException(String message) {
super(message);
}
}
7 changes: 5 additions & 2 deletions fe/fe-core/src/main/java/com/starrocks/persist/EditLog.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import com.google.gson.JsonParseException;
import com.starrocks.alter.AlterJobV2;
import com.starrocks.alter.BatchAlterJobPersistInfo;
import com.starrocks.authentication.UserAuthenticationInfo;
Expand Down Expand Up @@ -63,6 +64,7 @@
import com.starrocks.journal.JournalEntity;
import com.starrocks.journal.JournalInconsistentException;
import com.starrocks.journal.JournalTask;
import com.starrocks.journal.SerializeException;
import com.starrocks.journal.bdbje.Timestamp;
import com.starrocks.load.DeleteMgr;
import com.starrocks.load.ExportFailMsg;
Expand Down Expand Up @@ -1150,9 +1152,10 @@ private JournalTask submitLog(short op, Writable writable, long maxWaitIntervalM
entity.setOpCode(op);
entity.setData(writable);
entity.write(buffer);
} catch (IOException e) {
} catch (IOException | JsonParseException e) {
// The old implementation swallow exception like this
LOG.info("failed to serialize, ", e);
LOG.info("failed to serialize journal data", e);
throw new SerializeException("failed to serialize journal data");
}
JournalTask task = new JournalTask(startTimeNano, buffer, maxWaitIntervalMs);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@
import com.starrocks.common.util.concurrent.lock.Locker;
import com.starrocks.connector.ConnectorMetadata;
import com.starrocks.connector.exception.StarRocksConnectorException;
import com.starrocks.journal.SerializeException;
import com.starrocks.lake.DataCacheInfo;
import com.starrocks.lake.LakeMaterializedView;
import com.starrocks.lake.LakeTable;
Expand Down Expand Up @@ -1919,6 +1920,10 @@ public void onCreate(Database db, Table table, String storageVolumeId, boolean i
CreateTableInfo createTableInfo = new CreateTableInfo(db.getFullName(), table, storageVolumeId);
GlobalStateMgr.getCurrentState().getEditLog().logCreateTable(createTableInfo);
table.onCreate(db);
} catch (SerializeException e) {
db.unRegisterTableUnlocked(table);
LOG.warn("create table failed", e);
ErrorReport.reportDdlException(ErrorCode.ERR_CANT_CREATE_TABLE, table.getName(), e.getMessage());
} finally {
locker.unLockDatabase(db.getId(), LockType.WRITE);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static void beforeClass() throws Exception {
Config.alter_scheduler_interval_millisecond = 1000;
FeConstants.runningUnitTest = true;

UtFrameUtils.createMinStarRocksCluster();
UtFrameUtils.createMinStarRocksCluster(true, RunMode.SHARED_NOTHING);

// create connect context
connectContext = UtFrameUtils.createDefaultCtx();
Expand Down Expand Up @@ -311,4 +311,17 @@ public void testRenameColumnException() throws Exception {
}
}

@Test
public void testCreateTableSerializeException() {
final long tableId = 1000010L;
final String tableName = "test";
Database db = connectContext.getGlobalStateMgr().getLocalMetastore().getDb("test");
LocalMetastore localMetastore = connectContext.getGlobalStateMgr().getLocalMetastore();
SerializeFailedTable table = new SerializeFailedTable(1000010L, "serialize_test");

Assert.assertThrows(DdlException.class, () -> localMetastore.onCreate(db, table, "", true));

Assert.assertNull(db.getTable(tableId));
Assert.assertNull(db.getTable(tableName));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2021-present StarRocks, Inc. All rights reserved.
//
// Licensed 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
//
// https://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 com.starrocks.server;

import com.starrocks.catalog.Table;
import com.starrocks.persist.gson.GsonPreProcessable;

import java.io.DataOutput;
import java.io.IOException;
import java.util.ArrayList;

public class SerializeFailedTable extends Table implements GsonPreProcessable {

public SerializeFailedTable(long id, String name) {
super(id, name, TableType.OLAP, new ArrayList<>());
}

@Override
public void write(DataOutput out) throws IOException {
throw new IOException("failed");
}

@Override
public void gsonPreProcess() throws IOException {
throw new IOException("failed");
}
}
Loading