Skip to content

[ConfigNode] Fix RegionGroup creation race with database deletion#18273

Open
CRZbulabula wants to merge 1 commit into
apache:masterfrom
CRZbulabula:yongzao/fix-create-region-groups-database-race
Open

[ConfigNode] Fix RegionGroup creation race with database deletion#18273
CRZbulabula wants to merge 1 commit into
apache:masterfrom
CRZbulabula:yongzao/fix-create-region-groups-database-race

Conversation

@CRZbulabula

Copy link
Copy Markdown
Contributor

Description

CreateRegionGroupsProcedure could race with DeleteDatabaseProcedure. If the database partition table had already been removed when a replicated CreateRegionGroupsPlan was applied, PartitionInfo#createRegionGroups treated the database as not pre-deleted and dereferenced a missing DatabasePartitionTable, causing an NPE in the ConfigRegion state machine.

Simply skipping a missing database is unsafe because Region replicas may already have been created on DataNodes, and a delayed plan could be applied to a later database that reuses the same name.

This PR:

  • adds a monotonically increasing lifecycle generation for each database incarnation and records it in RegionGroup allocation plans, procedure state, and PartitionInfo snapshots;
  • validates the complete CreateRegionGroupsPlan batch before mutation and returns explicit statuses for missing, pre-deleted, or generation-mismatched databases;
  • serializes CreateRegionGroupsProcedure and DeleteDatabaseProcedure against each other with database-level lifecycle locks, including atomic lock acquisition for multi-database plans;
  • submits independent RemoveRegionGroupProcedure compensation for replicas that were created on DataNodes when RegionGroup persistence is rejected;
  • advances the RegionGroup ID high-water mark even for rejected replicated plans; and
  • keeps snapshot and procedure deserialization compatible with data written before lifecycle generations were introduced.

Tests

The added tests cover:

  • pre-deleted and fully removed databases;
  • rejection of a stale plan after same-name database recreation;
  • atomic validation of batched plans;
  • RegionGroup ID advancement on rejected plans;
  • snapshot, plan, and procedure serialization across recovery/leader changes;
  • mutual exclusion between RegionGroup creation and database deletion; and
  • compensation that removes only replicas successfully created on DataNodes.

Validation performed:

mvn spotless:apply -pl iotdb-core/confignode
mvn test -pl iotdb-core/confignode -Dtest=PartitionInfoTest,CreateRegionGroupsProcedureTest,ConfigPhysicalPlanSerDeTest
mvn compile -pl iotdb-core/confignode -DskipTests
mvn test-compile -DskipTests
mvn test-compile -P with-zh-locale -DskipTests

The targeted test run completed with 121 tests and no failures. Both full-reactor locale builds completed successfully.

This PR has:

  • been self-reviewed.
    • concurrent write
  • added comments explaining non-obvious concurrency and recovery behavior.
  • added or updated unit tests for the new code paths.
  • added integration tests.
  • been tested in a test IoTDB cluster.
Key changed/added classes
  • PartitionInfo
  • DatabasePartitionTable
  • CreateRegionGroupsPlan
  • CreateRegionGroupsProcedure
  • DeleteDatabaseProcedure
  • AbstractDatabaseProcedure

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens ConfigNode RegionGroup allocation against races with database deletion by introducing a per-database lifecycle generation fence, validating CreateRegionGroupsPlan batches before mutation, and serializing lifecycle-changing procedures with database-scoped locks. It also adds compensation procedures to clean up Region replicas created on DataNodes when persistence is rejected, and extends snapshot/procedure/plan serialization to remain compatible with pre-generation data.

Changes:

  • Introduce database lifecycle generations and propagate them through allocation plans, persistence validation, and PartitionInfo snapshots.
  • Add database-level lifecycle locks (including atomic multi-database acquisition) to serialize CreateRegionGroupsProcedure and DeleteDatabaseProcedure.
  • Add targeted unit tests for rejection/compensation, atomic validation, ID advancement on rejected plans, and serialization compatibility.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/procedure/impl/CreateRegionGroupsProcedureTest.java Adds tests for persist rejection cleanup and mutual exclusion between create/delete database lifecycle locks.
iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/persistence/PartitionInfoTest.java Adds tests for pre-deleted/missing database rejection, stale plan fencing after recreation, atomic batch validation, and region ID advancement.
iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/consensus/request/ConfigPhysicalPlanSerDeTest.java Adds SerDe coverage for CreateRegionGroupsPlan generation map and updates procedure SerDe expectations.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/scheduler/LockQueue.java Adds isIdle() to support removing unused per-database lock queues.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/schema/DeleteDatabaseProcedure.java Moves to AbstractDatabaseProcedure and declares database name(s) for lifecycle locking.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/region/CreateRegionGroupsProcedure.java Adds pre-validation, persists generation metadata, and submits cleanup procedures on persistence rejection.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/AbstractDatabaseProcedure.java New base procedure that holds exclusive lifecycle locks for its database set until completion.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/env/ConfigNodeProcedureEnv.java Adds per-database lock map and atomic multi-lock acquisition/release APIs; exposes plan validation.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/PartitionInfo.java Adds generation tracking, plan validation, snapshot format changes, and advances region group ID high-water mark on rejected plans.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/partition/DatabasePartitionTable.java Stores immutable database generation and includes it in equality/hash.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/partition/PartitionManager.java Exposes generation and plan validation to higher-level components.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/load/balancer/RegionBalancer.java Records current database generation into allocation plans.
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/consensus/request/write/region/CreateRegionGroupsPlan.java Adds generation map + SerDe support with backward compatibility.
iotdb-core/confignode/src/main/i18n/zh/org/apache/iotdb/confignode/i18n/ConfigNodeMessages.java Adds localized log/message constants for CreateRegionGroupsPlan rejection reasons.
iotdb-core/confignode/src/main/i18n/en/org/apache/iotdb/confignode/i18n/ConfigNodeMessages.java Adds localized log/message constants for CreateRegionGroupsPlan rejection reasons.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +249 to +252
final long expectedGeneration = plan.getDatabaseGeneration(database);
final long currentGeneration = databasePartitionTable.getDatabaseGeneration();
if (plan.isDatabaseGenerationSet(database) && expectedGeneration != currentGeneration) {
LOGGER.warn(
Comment on lines 1085 to +1088
// serialize nextRegionGroupId
ReadWriteIOUtils.write(SNAPSHOT_WITH_DATABASE_GENERATION_MAGIC, bufferedOutputStream);
ReadWriteIOUtils.write(nextRegionGroupId.get(), bufferedOutputStream);
ReadWriteIOUtils.write(nextDatabaseGeneration.get(), bufferedOutputStream);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants