Fix pipe consensus compatibility during rolling upgrade#17428
Fix pipe consensus compatibility during rolling upgrade#17428jt2594838 merged 3 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds rolling-upgrade compatibility for legacy pipe-consensus plugin/protocol names by introducing aliases and normalizing persisted consensus protocol class names so mixed-version clusters can restart cleanly.
Changes:
- Add builtin pipe plugin aliases for legacy
pipe-consensus-*processor/connector/sink names. - Normalize persisted IoTConsensusV2-related consensus protocol class names on DataNode/ConfigNode restart and rewrite system properties when needed.
- Extend DataNode plugin agent test coverage for legacy pipe-consensus names (processor/connector).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/agent/plugin/builtin/BuiltinPipePlugin.java |
Adds legacy pipe-consensus-* builtin plugin name aliases mapping to IoTConsensusV2 implementations. |
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/agent/plugin/dataregion/PipeDataRegionProcessorConstructor.java |
Registers the legacy processor alias so DataNode can instantiate the correct processor. |
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/agent/plugin/dataregion/PipeDataRegionSinkConstructor.java |
Registers the legacy async connector/sink aliases so DataNode can instantiate the correct sink. |
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/pipe/agent/plugin/PipeDataNodePluginAgentTest.java |
Adds assertions to validate legacy processor/connector alias resolution in the DataNode plugin agent. |
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBStartCheck.java |
Normalizes persisted DataNode consensus protocol class names and rewrites system properties for compatibility. |
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ConsensusFactory.java |
Adds normalization helper and applies it before consensus implementation construction. |
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/conf/SystemPropertiesUtils.java |
Normalizes persisted ConfigNode consensus protocol class names and rewrites system properties for compatibility. |
Comments suppressed due to low confidence (1)
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/ConsensusFactory.java:77
getConsensusImplnow normalizesclassName, but ifclassNameis null/blank the subsequentClass.forName(className)will throw aNullPointerException, which is not caught here (only checked exceptions are caught). Add an explicit null/blank guard after normalization that logs a clear error and returnsOptional.empty()to prevent startup crashes with confusing stack traces.
public static Optional<IConsensus> getConsensusImpl(
String className, ConsensusConfig config, IStateMachine.Registry registry) {
try {
className = normalizeConsensusProtocolClass(className);
// special judge for IoTConsensusV2
if (IOT_CONSENSUS_V2.equals(className)) {
className = REAL_IOT_CONSENSUS_V2;
// initialize iotConsensusV2's thrift component
IoTV2GlobalComponentContainer.build();
// initialize iotConsensusV2's metric component
IoTConsensusV2SyncLagManager.build();
}
Class<?> executor = Class.forName(className);
Constructor<?> executorConstructor =
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -136,9 +152,22 @@ public static void checkSystemProperties() throws IOException { | |||
| conf.setConfigNodeConsensusProtocolClass(configNodeConsensusProtocolClass); | |||
| } | |||
There was a problem hiding this comment.
In checkSystemProperties, persisted*ConsensusProtocolClass is read with a null default. If the key is missing (e.g., upgrading from an older/partial system properties file), normalizeConsensusProtocolClass(null) returns null, and the code will end up calling conf.set*ConsensusProtocolClass(null), which can later lead to a NullPointerException when constructing the consensus implementation. Consider falling back to the current conf value (or a known default) when the persisted value is null/blank, and avoid ever writing/setting a null consensus protocol class.
| BuiltinPipePlugin.PIPE_CONSENSUS_ASYNC_CONNECTOR.getPipePluginName()); | ||
| } | ||
| })) | ||
| .getClass()); |
There was a problem hiding this comment.
The new compatibility alias PIPE_CONSENSUS_ASYNC_SINK is added in the constructors, but the test only verifies the legacy processor and connector aliases. Consider adding an assertion that reflectSink also resolves when the sink key is set to BuiltinPipePlugin.PIPE_CONSENSUS_ASYNC_SINK.getPipePluginName() (i.e., cover the sink alias path, not just connector).
| .getClass()); | |
| .getClass()); | |
| Assert.assertEquals( | |
| IoTConsensusV2AsyncSink.class, | |
| agent | |
| .dataRegion() | |
| .reflectSink( | |
| new PipeParameters( | |
| new HashMap<String, String>() { | |
| { | |
| put( | |
| PipeSinkConstant.SINK_KEY, | |
| BuiltinPipePlugin.PIPE_CONSENSUS_ASYNC_SINK.getPipePluginName()); | |
| } | |
| })) | |
| .getClass()); |
Summary