*: Refine some comments of handling DDL and startup (#10439)

ref #6233

Signed-off-by: JaySon-Huang <tshent@qq.com>

Author: JaySon-Huang

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

@@ -375,9 +375,9 @@ void DAGStorageInterpreter::executeImpl(
     auto remote_requests = buildRemoteRequests(dag_context.scan_context_map[table_scan.getTableScanExecutorID()]);
     if (dag_context.is_disaggregated_task && !remote_requests.empty())
     {
-        // This means RN is sending requests with stale region info, we simply reject the request
-        // and ask RN to send requests again with correct region info. When RN updates region info,
-        // RN may be sending requests to other WN.
+        // This means compute node is sending requests with stale region info, we simply reject the request
+        // and ask compute node to send requests again with correct region info. When compute node updates region info,
+        // compute node may be sending requests to other WN.
 
         RegionException::UnavailableRegions region_ids;
         for (const auto & info : context.getDAGContext()->retry_regions)
@@ -487,9 +487,9 @@ void DAGStorageInterpreter::executeImpl(DAGPipeline & pipeline)
     auto remote_requests = buildRemoteRequests(dag_context.scan_context_map[table_scan.getTableScanExecutorID()]);
     if (dag_context.is_disaggregated_task && !remote_requests.empty())
     {
-        // This means RN is sending requests with stale region info, we simply reject the request
-        // and ask RN to send requests again with correct region info. When RN updates region info,
-        // RN may be sending requests to other WN.
+        // This means compute node is sending requests with stale region info, we simply reject the request
+        // and ask compute node to send requests again with correct region info. When compute node updates region info,
+        // compute node may be sending requests to other write node.
 
         RegionException::UnavailableRegions region_ids;
         for (const auto & info : context.getDAGContext()->retry_regions)
@@ -878,7 +878,7 @@ LearnerReadSnapshot DAGStorageInterpreter::doBatchCopLearnerRead()
         }
         catch (const LockException & e)
         {
-            // When this is a disaggregated read task on WN issued by RN, we need RN
+            // When this is a disaggregated read task on write node issued by compute node, we need compute node
             // to take care of retrying.
             if (context.getDAGContext()->is_disaggregated_task)
                 throw;
@@ -890,7 +890,7 @@ LearnerReadSnapshot DAGStorageInterpreter::doBatchCopLearnerRead()
         }
         catch (const RegionException & e)
         {
-            // When this is a disaggregated read task on WN issued by RN, we need RN
+            // When this is a disaggregated read task on write node issued by compute node, we need compute node
             // to take care of retrying.
             if (context.getDAGContext()->is_disaggregated_task)
                 throw;

dbms/src/Flash/Planner/Plans/PhysicalTableScan.cpp

@@ -101,6 +101,7 @@ void PhysicalTableScan::buildBlockInputStreamImpl(DAGPipeline & pipeline, Contex
 
     if (context.getSharedContextDisagg()->isDisaggregatedComputeMode())
     {
+        // Interact between compute node and write node.
         StorageDisaggregatedInterpreter disaggregated_tiflash_interpreter(
             context,
             tidb_table_scan,
@@ -124,6 +125,7 @@ void PhysicalTableScan::buildPipeline(
     // For building PipelineExec in compile time.
     if (context.getSharedContextDisagg()->isDisaggregatedComputeMode())
     {
+        // Interact between compute node and write node.
         StorageDisaggregatedInterpreter disaggregated_tiflash_interpreter(
             context,
             tidb_table_scan,

dbms/src/Server/Server.cpp

@@ -1138,6 +1138,7 @@ try
         SessionCleaner session_cleaner(*global_context);
         auto & tmt_context = global_context->getTMTContext();
 
+        // Start the proxy service, including the grpc service for raft and http status service
         proxy_machine.startProxyService(tmt_context, store_ident);
         if (proxy_machine.isProxyRunnable())
         {
@@ -1162,6 +1163,8 @@ try
                     syncSchemaWithTiDB(storage_config, bg_init_stores, terminate_signals_counter, global_context, log);
                     bg_init_stores.waitUntilFinish();
                 }
+
+                // Start read index workers and wait region apply index catch up with TiKV before serving requests.
                 proxy_machine.waitProxyServiceReady(tmt_context, terminate_signals_counter);
             }
         }
@@ -1224,13 +1227,13 @@ try
             GRPCCompletionQueuePool::global_instance = std::make_unique<GRPCCompletionQueuePool>(size);
         }
 
-        /// startup grpc server to serve raft and/or flash services.
+        /// startup flash service for handling coprocessor and MPP requests.
         FlashGrpcServerHolder flash_grpc_server_holder(this->context(), this->config(), raft_config, log);
 
         SCOPE_EXIT({
             // Stop LAC for AutoScaler managed CN before FlashGrpcServerHolder is destructed.
             // Because AutoScaler it will kill tiflash process when port of flash_server_addr is down.
-            // And we want to make sure LAC is cleanedup.
+            // And we want to make sure LAC is cleaned up.
             // The effects are there will be no resource control during [lac.safeStop(), FlashGrpcServer destruct done],
             // but it's basically ok, that duration is small(normally 100-200ms).
             if (is_disagg_compute_mode && disagg_opt.use_autoscaler && LocalAdmissionController::global_instance)

dbms/src/Storages/KVStore/ProxyStateMachine.h

@@ -114,7 +114,7 @@ struct TiFlashProxyConfig
 private:
     TiFlashProxyConfig()
     {
-        // For test, bootstrap no proxy.
+        // For test, bootstrap without proxy.
     }
     // TiFlash Proxy will set the default value of "flash.proxy.addr", so we don't need to set here.
     void addExtraArgs(const std::string & k, const std::string & v) { val_map["--" + k] = v; }
@@ -331,6 +331,10 @@ struct ProxyStateMachine
         tiflash_instance_wrap.tmt = &tmt_context;
         LOG_INFO(log, "Let tiflash proxy start all services");
         // Set tiflash instance status to running, then wait for proxy enter running status
+        // It means that in tiflash-proxy
+        // * rpc server for handling raft command is started
+        // * http status server is started
+        // https://github.com/pingcap/tidb-engine-ext/blob/74d6916e0aee34783cf3835b6fb93d40f32bb889/proxy_components/proxy_server/src/run.rs#L241-L265
         tiflash_instance_wrap.status = EngineStoreServerStatus::Running;
         while (tiflash_instance_wrap.proxy_helper->getProxyStatus() == RaftProxyStatus::Idle)
             std::this_thread::sleep_for(std::chrono::milliseconds(200));

dbms/src/Storages/KVStore/Read/ReadIndex.cpp

@@ -307,6 +307,11 @@ void WaitCheckRegionReadyImpl(
         total_regions_cnt);
 }
 
+/**
+ * For the region_ids stored in `kvstore`
+ * 1. fetch the latest commit-index from TiKV
+ * 2. wait until the tiflash-proxy apply raft logs for regions has catchup (applied to the commit-index)
+ */
 void WaitCheckRegionReady(KVStore & kvstore, const std::atomic_size_t & terminate_signals_counter)
 {
     const auto & config = kvstore.getConfigRef();

dbms/src/TiDB/Schema/DatabaseInfoCache.h

@@ -23,6 +23,7 @@
 namespace DB
 {
 
+// DatabaseInfoCache is a thread-safe cache for DatabaseID -> TiDB::DBInfo
 class DatabaseInfoCache
 {
 public:

dbms/src/TiDB/Schema/SchemaBuilder.cpp

@@ -370,7 +370,7 @@ void SchemaBuilder<Getter, NameMapper>::applyDiff(const SchemaDiff & diff)
         {
             // >= SchemaActionType::MaxRecognizedType
             // log down the Int8 value directly
-            LOG_ERROR(log, "Unsupported change type: {}, diff_version={}", fmt::underlying(diff.type), diff.version);
+            LOG_WARNING(log, "Unsupported change type: {}, diff_version={}", fmt::underlying(diff.type), diff.version);
         }
 
         break;
@@ -1138,7 +1138,7 @@ void SchemaBuilder<Getter, NameMapper>::applyCreateStorageInstance(
     // Else the storage instance does not exist, create it.
 
     // We need to create a Storage instance to handle its raft log and snapshot when it
-    // is "dropped" but not physically removed in TiDB. To handle it porperly, we get a
+    // is "dropped" but not physically removed in TiDB. To handle it properly, we get a
     // tso from PD to create the table. The tso must be newer than what "DROP TABLE" DDL
     // is executed. So when the gc-safepoint is larger than tombstone_ts, the table can
     // be safe to physically drop on TiFlash.
@@ -1434,7 +1434,7 @@ void SchemaBuilder<Getter, NameMapper>::syncAllSchema()
     }
     sync_all_schema_wait_group->wait();
 
-    // `applyRenameLogicalTable` is not atmoic when renaming a partitioned table
+    // `applyRenameLogicalTable` is not atomic when renaming a partitioned table
     // to new database. There could be a chance that the logical table .sql have
     // been moved to the new database while some partitions' sql are not moved.
     // Try to detect such situation and fix it.
@@ -1526,7 +1526,7 @@ void SchemaBuilder<Getter, NameMapper>::tryFixPartitionsBelongingDatabase()
             auto it = part_to_db_id.find(*opt_tbl_id);
             if (it == part_to_db_id.end())
             {
-                // this is not a physical_table_id of a partition, ignore sliently
+                // this is not a physical_table_id of a partition, ignore silently
                 continue;
             }
             // Get the `new_database_id` from `table_id_map`
@@ -1769,8 +1769,8 @@ String SchemaBuilder<Getter, NameMapper>::tryGetDatabaseDisplayNameFromLocal(Dat
 {
     // This method is called in the `applyDiff` loop. The `applyDiff` loop should apply the all the DDL operations before
     // the database get dropped, so the database info should be cached in `databases`.
-    // But for corner cases that the database is dropped on some unkonwn cases, we just return a display database name
-    // according to the keyspace_id and database_id because display name ususally is not critical.
+    // But for corner cases that the database is dropped on some unknown cases, we just return a display database name
+    // according to the keyspace_id and database_id because display name usually is not critical.
     if (auto new_db_info = databases.getDBInfo(database_id); likely(new_db_info != nullptr))
     {
         return name_mapper.displayDatabaseName(*new_db_info);

dbms/src/TiDB/Schema/SchemaBuilder.h

@@ -31,12 +31,14 @@ struct SchemaBuilder
 private:
     NameMapper name_mapper;
 
+    // Snapshot reader of schema info from TiKV
     Getter & getter;
 
     Context & context;
 
+    // Cache of DatabaseID -> DatabaseInfo in this keyspace
     DatabaseInfoCache & databases;
-
+    // Cache of TableIDMap in this keyspace
     TableIDMap & table_id_map;
 
     const KeyspaceID keyspace_id;

dbms/src/TiDB/Schema/SchemaGetter.h

@@ -28,7 +28,7 @@
 namespace DB
 {
 // The enum results are completely the same as the DDL Action listed in the "parser/model/ddl.go" of TiDB codebase, which must be keeping in sync.
-// https://github.com/pingcap/tidb/blob/84492a9a1e5bff0b4a4256955ab8231975c2dde1/pkg/meta/model/job.go#L35-L36
+// https://github.com/pingcap/tidb/blob/35094ab95c393e6d55b00a215d24e859880ec1f5/pkg/meta/model/job.go#L36-L120
 enum class SchemaActionType : Int8
 {
     None = 0,
@@ -105,11 +105,14 @@ enum class SchemaActionType : Int8
     ActionRemovePartitioning = 72,
     ActionAddColumnarIndex = 73,
     ActionModifyEngineAttribute = 74,
+    ActionAlterTableMode = 75,
+    ActionRefreshMeta = 76,
+    ActionModifySchemaReadOnly = 77,
 
     // If we support new type from TiDB.
     // MaxRecognizedType also needs to be changed.
     // It should always be equal to the maximum supported type + 1
-    MaxRecognizedType = 75,
+    MaxRecognizedType = 78,
 };
 
 struct AffectedOption

dbms/src/TiDB/Schema/SchemaSyncer.h

@@ -16,18 +16,10 @@
 
 #include <Interpreters/Context_fwd.h>
 #include <Storages/KVStore/Types.h>
+#include <TiDB/Schema/TiDB_fwd.h>
 #include <common/logger_useful.h>
 
 #include <memory>
-#include <vector>
-
-namespace TiDB
-{
-struct DBInfo;
-using DBInfoPtr = std::shared_ptr<DBInfo>;
-struct TableInfo;
-using TableInfoPtr = std::shared_ptr<TableInfo>;
-} // namespace TiDB
 
 namespace DB
 {
@@ -48,17 +40,25 @@ class SchemaSyncer
      */
     virtual bool syncTableSchema(Context & context, TableID physical_table_id) = 0;
 
-    virtual void reset() = 0;
-
-    virtual TiDB::DBInfoPtr getDBInfoByName(const String & database_name) = 0;
-
+    /*
+     * When the table is physically dropped from the TiFlash node, use this method to unregister
+     * the TableID mapping.
+     */
     virtual void removeTableID(TableID table_id) = 0;
 
     /**
       * Drop all schema of a given keyspace.
       * When a keyspace is removed, drop all its databases and tables.
       */
     virtual void dropAllSchema(Context & context) = 0;
+
+    /*
+     * Clear all states.
+     * just for testing restart
+     */
+    virtual void reset() = 0;
+
+    virtual TiDB::DBInfoPtr getDBInfoByName(const String & database_name) = 0;
 };
 
 using SchemaSyncerPtr = std::shared_ptr<SchemaSyncer>;