review fixes

Signed-off-by: Evgeniy Zuykin <[email protected]>
StarRocks · Jan 13, 2025 · ce900f3 · ce900f3
1 parent 3a8e612
commit ce900f3
Show file tree

Hide file tree

Showing 9 changed files with 32 additions and 50 deletions.
diff --git a/fe/fe-core/src/main/java/com/starrocks/meta/SqlBlackList.java b/fe/fe-core/src/main/java/com/starrocks/meta/SqlBlackList.java
@@ -19,7 +19,7 @@
 import com.starrocks.common.AnalysisException;
 import com.starrocks.common.ErrorCode;
 import com.starrocks.common.ErrorReport;
-import com.starrocks.persist.AddSqlBlackList;
+import com.starrocks.persist.SqlBlackListPersistInfo;
 import com.starrocks.persist.ImageWriter;
 import com.starrocks.persist.metablock.SRMetaBlockEOFException;
 import com.starrocks.persist.metablock.SRMetaBlockException;
@@ -63,8 +63,8 @@ public void load(SRMetaBlockReader reader) throws IOException, SRMetaBlockExcept
         try (LockCloseable ignored = new LockCloseable(rwLock.writeLock())) {
             int cnt = reader.readInt();
             for (int i = 0; i < cnt; i++) {
-                AddSqlBlackList addSqlBlackList = reader.readJson(AddSqlBlackList.class);
-                put(addSqlBlackList.id, Pattern.compile(addSqlBlackList.pattern));
+                SqlBlackListPersistInfo sqlBlackListPersistInfo = reader.readJson(SqlBlackListPersistInfo.class);
+                put(sqlBlackListPersistInfo.id, Pattern.compile(sqlBlackListPersistInfo.pattern));
             }
             LOG.info("loaded {} SQL blacklist patterns", sqlBlackListMap.size());
         }
@@ -105,6 +105,12 @@ public void delete(long id) {
         }
     }
 
+    public void delete(List<Long> ids) {
+        for (Long id : ids) {
+            this.delete(id);
+        }
+    }
+
     public void save(ImageWriter imageWriter) throws IOException, SRMetaBlockException {
         try (LockCloseable ignored = new LockCloseable(rwLock.readLock())) {
             // one for self and N for patterns
@@ -114,7 +120,7 @@ public void save(ImageWriter imageWriter) throws IOException, SRMetaBlockExcepti
             // write patterns
             writer.writeInt(sqlBlackListMap.size());
             for (BlackListSql p : sqlBlackListMap.values()) {
-                writer.writeJson(new AddSqlBlackList(p.id, p.pattern.pattern()));
+                writer.writeJson(new SqlBlackListPersistInfo(p.id, p.pattern.pattern()));
             }
             writer.close();
         }

diff --git a/fe/fe-core/src/main/java/com/starrocks/persist/EditLog.java b/fe/fe-core/src/main/java/com/starrocks/persist/EditLog.java
@@ -1113,16 +1113,14 @@ public void loadJournal(GlobalStateMgr globalStateMgr, JournalEntity journal)
                     break;
                 }
                 case OperationType.OP_ADD_SQL_QUERY_BLACK_LIST: {
-                    AddSqlBlackList addBlacklistRequest = (AddSqlBlackList) journal.data();
+                    SqlBlackListPersistInfo addBlacklistRequest = (SqlBlackListPersistInfo) journal.data();
                     GlobalStateMgr.getCurrentState().getSqlBlackList()
                             .put(addBlacklistRequest.id, Pattern.compile(addBlacklistRequest.pattern));
                     break;
                 }
                 case OperationType.OP_DELETE_SQL_QUERY_BLACK_LIST: {
                     DeleteSqlBlackLists deleteBlackListsRequest = (DeleteSqlBlackLists) journal.data();
-                    for (int i = 0; i < deleteBlackListsRequest.ids.size(); i++) {
-                        GlobalStateMgr.getCurrentState().getSqlBlackList().delete(deleteBlackListsRequest.ids.get(i));
-                    }
+                    GlobalStateMgr.getCurrentState().getSqlBlackList().delete(deleteBlackListsRequest.ids);
                     break;
                 }
                 default: {
@@ -1809,7 +1807,7 @@ public void logAlterMaterializedViewProperties(ModifyTablePropertyOperationLog l
         logEdit(OperationType.OP_ALTER_MATERIALIZED_VIEW_PROPERTIES, log);
     }
 
-    public void logAddSQLBlackList(AddSqlBlackList addBlackList) {
+    public void logAddSQLBlackList(SqlBlackListPersistInfo addBlackList) {
         logEdit(OperationType.OP_ADD_SQL_QUERY_BLACK_LIST, addBlackList);
     }
 

diff --git a/fe/fe-core/src/main/java/com/starrocks/persist/EditLogDeserializer.java b/fe/fe-core/src/main/java/com/starrocks/persist/EditLogDeserializer.java
@@ -243,7 +243,7 @@ public class EditLogDeserializer {
             .put(OperationType.OP_ALTER_WAREHOUSE, Warehouse.class)
             .put(OperationType.OP_DROP_WAREHOUSE, DropWarehouseLog.class)
             .put(OperationType.OP_CLUSTER_SNAPSHOT_LOG, ClusterSnapshotLog.class)
-            .put(OperationType.OP_ADD_SQL_QUERY_BLACK_LIST, AddSqlBlackList.class)
+            .put(OperationType.OP_ADD_SQL_QUERY_BLACK_LIST, SqlBlackListPersistInfo.class)
             .put(OperationType.OP_DELETE_SQL_QUERY_BLACK_LIST, DeleteSqlBlackLists.class)
             .build();
 

diff --git a/fe/fe-core/src/main/java/com/starrocks/persist/OperationType.java b/fe/fe-core/src/main/java/com/starrocks/persist/OperationType.java
@@ -592,9 +592,9 @@ public class OperationType {
     public static final short OP_CLUSTER_SNAPSHOT_LOG = 13513;
 
     @IgnorableOnReplayFailed
-    public static final short OP_ADD_SQL_QUERY_BLACK_LIST = 13700;
+    public static final short OP_ADD_SQL_QUERY_BLACK_LIST = 13520;
     @IgnorableOnReplayFailed
-    public static final short OP_DELETE_SQL_QUERY_BLACK_LIST = 13701;
+    public static final short OP_DELETE_SQL_QUERY_BLACK_LIST = 13521;
 
     /**
      * NOTICE: OperationType cannot use a value exceeding 20000, please follow the above sequence number

diff --git a/...om/starrocks/persist/AddSqlBlackList.java → ...ocks/persist/SqlBlackListPersistInfo.java b/...om/starrocks/persist/AddSqlBlackList.java → ...ocks/persist/SqlBlackListPersistInfo.java
@@ -19,8 +19,8 @@
 
 import java.util.Objects;
 
-public class AddSqlBlackList extends JsonWriter {
-    public AddSqlBlackList(long id, String pattern) {
+public class SqlBlackListPersistInfo extends JsonWriter {
+    public SqlBlackListPersistInfo(long id, String pattern) {
         this.id = id;
         this.pattern = pattern;
     }
@@ -39,7 +39,7 @@ public boolean equals(Object o) {
         if (o == null || getClass() != o.getClass()) {
             return false;
         }
-        AddSqlBlackList that = (AddSqlBlackList) o;
+        SqlBlackListPersistInfo that = (SqlBlackListPersistInfo) o;
         return id == that.id && Objects.equals(pattern, that.pattern);
     }
 

diff --git a/fe/fe-core/src/main/java/com/starrocks/qe/StmtExecutor.java b/fe/fe-core/src/main/java/com/starrocks/qe/StmtExecutor.java
@@ -106,7 +106,7 @@
 import com.starrocks.mysql.MysqlCommand;
 import com.starrocks.mysql.MysqlEofPacket;
 import com.starrocks.mysql.MysqlSerializer;
-import com.starrocks.persist.AddSqlBlackList;
+import com.starrocks.persist.SqlBlackListPersistInfo;
 import com.starrocks.persist.CreateInsertOverwriteJobLog;
 import com.starrocks.persist.DeleteSqlBlackLists;
 import com.starrocks.persist.gson.GsonUtils;
@@ -446,11 +446,6 @@ private boolean initForwardToLeaderState() {
             }
         }
 
-        // always redirect sql blacklist related queries to the leader
-        if (parsedStmt instanceof AddSqlBlackListStmt || parsedStmt instanceof DelSqlBlackListStmt) {
-            return true;
-        }
-
         if (redirectStatus == null) {
             return false;
         } else {
@@ -1699,7 +1694,7 @@ private void handleAddSqlBlackListStmt() {
         AddSqlBlackListStmt addSqlBlackListStmt = (AddSqlBlackListStmt) parsedStmt;
         long id = GlobalStateMgr.getCurrentState().getSqlBlackList().put(addSqlBlackListStmt.getSqlPattern());
         GlobalStateMgr.getCurrentState().getEditLog()
-                .logAddSQLBlackList(new AddSqlBlackList(id, addSqlBlackListStmt.getSqlPattern().pattern()));
+                .logAddSQLBlackList(new SqlBlackListPersistInfo(id, addSqlBlackListStmt.getSqlPattern().pattern()));
     }
 
     private void handleDelSqlBlackListStmt() {

diff --git a/fe/fe-core/src/main/java/com/starrocks/sql/ast/AddSqlBlackListStmt.java b/fe/fe-core/src/main/java/com/starrocks/sql/ast/AddSqlBlackListStmt.java
@@ -65,7 +65,7 @@ public <R, C> R accept(AstVisitor<R, C> visitor, C context) {
 
     @Override
     public RedirectStatus getRedirectStatus() {
-        return RedirectStatus.NO_FORWARD;
+        return RedirectStatus.FORWARD_NO_SYNC;
     }
 }
 
diff --git a/fe/fe-core/src/main/java/com/starrocks/sql/ast/DelSqlBlackListStmt.java b/fe/fe-core/src/main/java/com/starrocks/sql/ast/DelSqlBlackListStmt.java
@@ -46,7 +46,7 @@ public <R, C> R accept(AstVisitor<R, C> visitor, C context) {
 
     @Override
     public RedirectStatus getRedirectStatus() {
-        return RedirectStatus.NO_FORWARD;
+        return RedirectStatus.FORWARD_NO_SYNC;
     }
 }
 
diff --git a/fe/fe-core/src/test/java/com/starrocks/server/SqlBlacklistTest.java b/fe/fe-core/src/test/java/com/starrocks/server/SqlBlacklistTest.java
@@ -14,15 +14,16 @@
 
 package com.starrocks.server;
 
-import com.starrocks.ha.FrontendNodeType;
+import com.starrocks.analysis.RedirectStatus;
 import com.starrocks.meta.BlackListSql;
 import com.starrocks.meta.SqlBlackList;
-import com.starrocks.persist.AddSqlBlackList;
+import com.starrocks.persist.SqlBlackListPersistInfo;
 import com.starrocks.persist.DeleteSqlBlackLists;
 import com.starrocks.persist.EditLog;
 import com.starrocks.qe.ConnectContext;
 import com.starrocks.qe.StmtExecutor;
 import com.starrocks.sql.analyzer.AnalyzeTestUtil;
+import com.starrocks.sql.ast.AddSqlBlackListStmt;
 import com.starrocks.sql.ast.DelSqlBlackListStmt;
 import com.starrocks.sql.ast.StatementBase;
 import com.starrocks.utframe.UtFrameUtils;
@@ -68,7 +69,7 @@ public EditLog getEditLog() {
 
         new Expectations(editLog) {
             {
-                editLog.logAddSQLBlackList(new AddSqlBlackList(0, ".+"));
+                editLog.logAddSQLBlackList(new SqlBlackListPersistInfo(0, ".+"));
                 times = 1;
             }
         };
@@ -120,27 +121,9 @@ public EditLog getEditLog() {
     }
 
     @Test
-    public void testRedirectBlacklistOperationIfNotLeader() {
-        MockUp<GlobalStateMgr> mockUp = new MockUp<GlobalStateMgr>() {
-            @Mock
-            public FrontendNodeType getFeType() {
-                return FrontendNodeType.FOLLOWER;
-            }
-
-            @Mock
-            public boolean isLeader() {
-                return false;
-            }
-        };
-        ConnectContext connectContext = UtFrameUtils.createDefaultCtx();
-
-        StatementBase addStatement = parseSql("ADD SQLBLACKLIST \".+\";");
-        StmtExecutor addStatementExecutor = new StmtExecutor(connectContext, addStatement);
-        Assert.assertTrue(addStatementExecutor.isForwardToLeader());
-
-        StatementBase deleteStatement = parseSql("DELETE SQLBLACKLIST 1,2,3;");
-        StmtExecutor deleteStatementExecutor = new StmtExecutor(connectContext, deleteStatement);
-        Assert.assertTrue(deleteStatementExecutor.isForwardToLeader());
+    public void testRedirectStatus() {
+        Assert.assertEquals(new AddSqlBlackListStmt("ADD SQLBLACKLIST \".+\";").getRedirectStatus(), RedirectStatus.FORWARD_NO_SYNC);
+        Assert.assertEquals(new DelSqlBlackListStmt(List.of(1L, 2L)).getRedirectStatus(), RedirectStatus.FORWARD_NO_SYNC);
     }
 
     @Test
@@ -164,8 +147,8 @@ public void testSqlBlacklistJournalOperations() throws Exception {
 
         // add blacklists
 
-        GlobalStateMgr.getCurrentState().getEditLog().logAddSQLBlackList(new AddSqlBlackList(123, "p1"));
-        GlobalStateMgr.getCurrentState().getEditLog().logAddSQLBlackList(new AddSqlBlackList(1234, "p2"));
+        GlobalStateMgr.getCurrentState().getEditLog().logAddSQLBlackList(new SqlBlackListPersistInfo(123, "p1"));
+        GlobalStateMgr.getCurrentState().getEditLog().logAddSQLBlackList(new SqlBlackListPersistInfo(1234, "p2"));
         UtFrameUtils.PseudoJournalReplayer.replayJournalToEnd();
 
         Assertions.assertIterableEquals(