Skip to content

[lake/iceberg] Fix listener leak in IcebergLakeCommitter#3542

Merged
luoyuxia merged 2 commits into
apache:mainfrom
Guosmilesmile:fix_listener_leak_in_iceberg
Jun 30, 2026
Merged

[lake/iceberg] Fix listener leak in IcebergLakeCommitter#3542
luoyuxia merged 2 commits into
apache:mainfrom
Guosmilesmile:fix_listener_leak_in_iceberg

Conversation

@Guosmilesmile

@Guosmilesmile Guosmilesmile commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Purpose

Because Iceberg's Listeners registry is:

  • JVM-global (a static Map<Class, Queue<Listener>>),
  • append-only (no deregister / unregister API),

every newly created IcebergLakeCommitter permanently appended one more
listener to the global queue. In a long-running tiering job, the queue keeps
growing and every CreateSnapshotEvent is dispatched to all of them,
causing:

  1. Memory leak – listener instances are never garbage-collected as long
    as the Listeners class is loaded.
  2. CPU overhead – each Iceberg commit triggers N listener notifications,
    where N grows over time.
  3. Correctness risk for shared catalogs – when multiple IcebergLakeCommitter instances live in the same JVM (e.g. multiple tiering tasks), all of their listeners write to the same ThreadLocal<Long> currentCommitSnapshotId. The last writer wins, but the duplicated work is still wasted.

Brief change log

  • Move listener registration from the constructor to a static {} block in
    IcebergLakeCommitter, so the listener is registered exactly once per
    ClassLoader
    (effectively once per JVM).
  • Keep the ThreadLocal#remove() call after each commit so no thread-local
    entry is leaked across commits.
  • Add IcebergLakeCommitterListenerLeakTest which:
    • Reflectively reads the private Listeners.LISTENERS registry.
    • Creates multiple IcebergLakeCommitter instances.
    • Asserts that the number of IcebergSnapshotCreateListener instances in the registry stays at exactly 1, regardless of how many committers are constructed.

Tests

The behavior is already coverd by existing test.

API and Format

No public API or storage format change.

Documentation

No documentation change; behavior-only fix.

@Guosmilesmile Guosmilesmile changed the title [lake/iceberg] Fix listener leak in IcebergLakeCommitter by registering snapshot listener only once chrisyguo A minute ago [lake/iceberg] Fix listener leak in IcebergLakeCommitter by registering snapshot listener only once Jun 29, 2026
@Guosmilesmile Guosmilesmile changed the title [lake/iceberg] Fix listener leak in IcebergLakeCommitter by registering snapshot listener only once [lake/iceberg] Fix listener leak in IcebergLakeCommitter Jun 29, 2026
@luoyuxia luoyuxia requested review from MehulBatra and Copilot June 29, 2026 08:54

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@luoyuxia luoyuxia 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.

@Guosmilesmile Thanks for the pr. lgtm overall. I only left one comment

import static org.apache.fluss.lake.iceberg.utils.IcebergConversions.toIceberg;
import static org.assertj.core.api.Assertions.assertThat;

class IcebergLakeCommitterListenerLeakTest {

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.

Can we just remove this test since it's a trival change and the behavior is already coverd by existing test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, remove IcebergLakeCommitterListenerLeakTest.

@luoyuxia luoyuxia 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.

Thanks. LGTM

@luoyuxia luoyuxia merged commit e426f40 into apache:main Jun 30, 2026
7 checks passed
@Guosmilesmile Guosmilesmile deleted the fix_listener_leak_in_iceberg branch June 30, 2026 07:58
@Guosmilesmile

Copy link
Copy Markdown
Contributor Author

Thanks @luoyuxia for reviewing !

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.

3 participants