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<string, set<string>> fetchTableMap)` was changed to
`fetchTables(map<string, set<string>> 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]
