Skip to content

Fix delayed operator memory release after fragment termination - #18387

Open
Caideyipi wants to merge 1 commit into
apache:masterfrom
Caideyipi:fix/select-into-memory-estimation
Open

Fix delayed operator memory release after fragment termination#18387
Caideyipi wants to merge 1 commit into
apache:masterfrom
Caideyipi:fix/select-into-memory-estimation

Conversation

@Caideyipi

Copy link
Copy Markdown
Collaborator

Description

Problem

Fragment-instance state listeners run on a shared bounded notification executor. The operator-memory estimate reserved by LocalExecutionPlanner was returned from one of those asynchronous listeners.

A fragment can report a fast execution failure, such as a SELECT INTO type mismatch, while notification threads are still closing drivers for earlier fragments. Consecutive failures can therefore accumulate completed fragments' admission reservations and eventually fail with a false memory-insufficient error.

Design

  • Add FragmentInstanceStateMachine.addFinalStateChangeListener for short, non-blocking bookkeeping that must finish in the thread performing the terminal transition. It invokes each pre-registered listener exactly once and invokes late registrations immediately.
  • Return the operator-memory estimate through this synchronous final-state listener.
  • Move the existing lightweight fragment statistics and DataNodeQueryContext cleanup to the same path.
  • Keep driver closing, file-handle release, exchange-pool deregistration, and other potentially blocking cleanup on the existing asynchronous listener.

This fixes all fast fragment termination paths (finished, failed, canceled, and aborted), rather than special-casing SELECT INTO or increasing test memory.

Tests

  • mvn spotless:check -pl iotdb-core/datanode
  • FragmentInstanceStateMachineTest
  • FragmentInstanceContextTest
  • LocalExecutionPlannerOperatorsMemoryTest

The three unit-test classes pass 20 tests. The new regression test deliberately uses a notification executor that never runs and verifies that terminal bookkeeping and operator-memory release still complete synchronously.


This PR has:

  • been self-reviewed.
    • concurrent read and write
  • added Javadocs for the new non-trivial method.
  • added comments explaining why synchronous bookkeeping is required.
  • added unit tests covering the new code paths.

Key changed/added classes
  • FragmentInstanceStateMachine
  • FragmentInstanceContext
  • LocalExecutionPlanner

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.

1 participant