Re: [PR] Cyb/metadata table [iotdb]
JackieTien97 merged PR #18127: URL: https://github.com/apache/iotdb/pull/18127 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Cyb/metadata table [iotdb]
Caideyipi commented on PR #18127:
URL: https://github.com/apache/iotdb/pull/18127#issuecomment-4901444079
I reviewed this PR against the HA requirement/design docs. The direction
looks good, but I found several issues that should be addressed before merge:
1. The DataNode fencing decision is not checked synchronously on access.
`MetadataLeaseManager.isFenced()` only checks whether `metadataState !=
NORMAL`. Lease expiration is moved from `NORMAL` to `NEED_CLEAR` only by the
scheduled `checkLeaseStatus()` task. If `check_dn_lease_status_interval_ms` is
configured larger than the ConfigNode-side `T_fence + margin`, the ConfigNode
may proceed with a metadata change while the partitioned DataNode is still in
`NORMAL` and can keep serving stale cache. Please make `isFenced()` /
`failIfMetadataLeaseFenced()` check the elapsed heartbeat age and CAS the state
to `NEED_CLEAR` immediately when the lease is expired.
2. A reachable but already-fenced DataNode can make the HA broadcast fail.
`DataNodeInternalRPCServiceImpl.updateTable()` calls
`DataNodeTableCache.preUpdateTable()`, which throws
`MetadataLeaseFencedException` while the DataNode is fenced. On the ConfigNode
side, `ClusterCachePropagator` treats any response other than `SUCCESS_STATUS`
and `CAN_NOT_CONNECT_DATANODE` as `FAIL`. This means a DataNode that has
correctly self-fenced but is still reachable, or is recovering in `NEED_CLEAR`
/ `PULLING`, can fail the DDL instead of being considered safe. Please
return/handle `METADATA_LEASE_FENCED` explicitly and let the propagator treat
it as a safe fenced response.
3. The advertised `supportsFencing` compatibility guard is missing.
The design says the ConfigNode should track whether each DataNode supports
self-fencing, with default `false` for old nodes. Current
`ClusterCachePropagator` only uses heartbeat age. During rolling upgrade or
mixed-version operation, an old DataNode that does not self-fence can still be
considered safe after `T_proceed`, which is unsafe. Please add capability
reporting/tracking and only skip unreachable DataNodes that are known to
support fencing.
4. The `fetchTables` Thrift change is not backward compatible.
`fetchTables(map> fetchTableMap)` was changed to
`fetchTables(map> fetchTableMap, byte tableNodeStatus)`.
This breaks old DataNode/new ConfigNode compatibility: an old caller will not
set the new argument, and the server can parse it as the default byte value `0`
(`PRE_CREATE`), which then goes into the unsupported branch in
`ConfigManager.fetchTables()`. Please use a new RPC or an optional/defaulted
argument that preserves the old behavior as `USING`.
5. There are new raw English log/message literals in main code.
For example, `ConfigNodeRPCServiceProcessor.reloadCacheAfterLeaseRecovery()`
logs `Execute getMetaDataCache with result {}` directly. This repo requires
user/operator visible strings to go through the i18n message constants. Please
move the new literals to the proper message classes.
Overall, the main concept is sound, but the above issues affect the safety
and compatibility of the lease-based HA mechanism.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Cyb/metadata table [iotdb]
JackieTien97 commented on code in PR #18127:
URL: https://github.com/apache/iotdb/pull/18127#discussion_r3534093133
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/RemoveDataNodeHandler.java:
##
@@ -455,6 +456,9 @@ public void
removeDataNodePersistence(List removedDataNodes)
PartitionMetrics.unbindDataNodePartitionMetricsWhenUpdate(
MetricService.getInstance(),
NodeUrlUtils.convertTEndPointUrl(dataNodeLocation.getClientRpcEndPoint()));
+ // Drop the removed DataNode's metadata-lease contact/capability state
so it is not retained,
+ // and a future DataNode reusing the id cannot inherit stale fencing
history.
Review Comment:
id will always increase, no new DN will reuse removed DN's id
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ProcedureManager.java:
##
@@ -2416,12 +2416,17 @@ public TDeleteTableDeviceResp deleteDevices(
}
}
+ // only care about the AbstractAlterOrDropTableProcedure(except the drop
table/view)
+ // and the DeleteDatabaseProcedure
Review Comment:
explain why we need to exclude the drop table/view procedure
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/schema/table/AbstractAlterOrDropTableProcedure.java:
##
@@ -91,17 +93,22 @@ protected void preRelease(final ConfigNodeProcedureEnv env)
{
}
protected void preRelease(final ConfigNodeProcedureEnv env, final @Nullable
String oldName) {
-final Map failedResults =
-SchemaUtils.preReleaseTable(database, table, env.getConfigManager(),
oldName);
-
-if (!failedResults.isEmpty()) {
- // All dataNodes must clear the related schema cache
+// Proceed once every unreachable DataNode is provably self-fenced instead
of hard-failing the
+// DDL: a fenced DataNode fails closed on its now-stale table cache and
resyncs on lease
+// recovery, so it cannot serve dirty schema. Only fail if an unacked
DataNode is not provably
+// fenced (it may still be serving clients).
+final TUpdateTableReq req = SchemaUtils.BuildPreUpdateTableReq(database,
table, oldName);
Review Comment:
```suggestion
final TUpdateTableReq req = SchemaUtils.buildPreUpdateTableReq(database,
table, oldName);
```
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/schema/table/AbstractAlterOrDropTableProcedure.java:
##
@@ -140,18 +147,21 @@ protected void rollbackPreRelease(final
ConfigNodeProcedureEnv env) {
protected void rollbackPreRelease(
final ConfigNodeProcedureEnv env, final @Nullable String tableName) {
-final Map failedResults =
-SchemaUtils.rollbackPreRelease(
-database, table.getTableName(), env.getConfigManager(), tableName);
-
-if (!failedResults.isEmpty()) {
- // All dataNodes must clear the related schema cache
+// A down DataNode must not block rollback either: proceed past
provably-fenced DataNodes (which
+// resync on recovery) and only fail on an unacked DataNode that is not
provably fenced.
+final TUpdateTableReq req =
+SchemaUtils.rollbackUpdateTableReq(database, table.getTableName(),
tableName);
+final boolean proceeded =
+new ClusterCachePropagator(env.getConfigManager())
+.propagate(targets -> SchemaUtils.broadcastTableUpdate(req,
targets));
+
+if (!proceeded) {
LOGGER.warn(
ProcedureMessages.FAILED_TO_ROLLBACK_PRE_RELEASE_FOR_TABLE_INFO_TO_DATANODE,
getActionMessage(),
database,
table.getTableName(),
- failedResults);
+ "an unreachable DataNode is not provably fenced");
Review Comment:
follow the i18n standarn, define a constant in both zh and en.
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/schema/table/CreateTableProcedure.java:
##
@@ -151,16 +153,22 @@ protected void preCreateTable(final
ConfigNodeProcedureEnv env) {
}
private void preReleaseTable(final ConfigNodeProcedureEnv env) {
-final Map failedResults =
-SchemaUtils.preReleaseTable(database, table, env.getConfigManager(),
null);
-
-if (!failedResults.isEmpty()) {
- // All dataNodes must clear the related schema cache
+// Broadcast the pre-update to all DataNodes. Instead of failing whenever
any DataNode is
+// unreachable, proceed once every unacked DataNode is provably
self-fenced: such a DataNode
+// fails closed on its (now-stale) table cache and resyncs on lease
recovery, so it cannot serve
+// dirty schema. Only fail if an unacked DataNode is not provably fenced
(it may still be
+// serving clients).
+final TUpdateTableReq req = SchemaUtils.BuildPreUpdateTableReq(database,
table, null);
+final boolean proceeded =
+new ClusterCachePropagator(env.getConfigManager())
+.
[PR] Cyb/metadata table [iotdb]
alpass163gmail opened a new pull request, #18127: URL: https://github.com/apache/iotdb/pull/18127 This PR introduces a lease-based self-fencing framework to enable high availability for table metadata operations. It prevents DataNodes from serving stale schema during network partitions. Problem Currently, in a cluster deployment (e.g., 1 ConfigNode, 3 DataNodes), metadata operations lack true high availability. If a single DataNode (DN) crashes or experiences a network partition, execution of DDL procedures (such as Create/Alter/Drop Table, View management, TTL adjustments, and Drop Database) will directly fail and trigger a rollback. Solution Lease Mechanism - MetadataLeaseManager (DataNode): Tracks lease via ConfigNode heartbeats. Uses monotonic clock. Self-fences (clears cache, blocks reads/writes) if no heartbeat received within metadata_lease_fence_ms (T_fence). - MetadataLeaseFencedException: Thrown when operations are blocked on a fenced DataNode. Broadcast Coordination - DataNodeContactTracker (ConfigNode): Records time of last successful heartbeat response per DataNode. Separately maintained from load-balancing samples to ensure correctness. - MetadataBroadcastVerdict: Pure decision logic — PROCEED if all unacked DataNodes have been silent for T_proceed = T_fence + margin, WAIT otherwise, FAIL when retry budget exhausted. - ClusterCachePropagator: Broadcasts cache invalidations with retry loop, waiting up to T_proceed for unresponsive DataNodes to prove self-fenced. Schema Change Integration - Procedures now propagate metadata invalidations via ClusterCachePropagator before proceeding. - Pre-deletion marker (PreDeleteTsTable) added for safe table state transitions. - Rollback mechanism (RollbackPreDeleteTablePlan) for failed schema changes. Configuration - New config: metadata_lease_fence_ms (default in iotdb-system.properties.template). Testing - Unit tests for MetadataLeaseManager, DataNodeContactTracker, MetadataBroadcastVerdict, ClusterCachePropagator - Lease integration tests for DataNodeTableCache, PartitionCache, ClusterAuthorityFetcher - New HA IT: IoTDBTableDDLHAIT Key Components Added - MetadataLeaseManager (DataNode): Tracks the lease and performs self-fencing upon expiration. - DataNodeContactTracker (ConfigNode): Tracks the last successful heartbeat timestamp for each DataNode. - ClusterCachePropagator (ConfigNode): Broadcasts cache invalidations with a fencing-aware retry mechanism. - MetadataBroadcastVerdict (ConfigNode): Decides when unacknowledged DataNodes are logically safe to skip. - MetadataLeaseFencedException (node-commons): The explicit exception thrown on a fenced DataNode. - PreDeleteTsTable (node-commons): Represents the marker table state to ensure safe schema deletion. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
