Skip to content

[fix][broker] Stop reporting a deliberate ownership release as an expired resource lock - #26533

Merged
merlimat merged 1 commit into
apache:masterfrom
SongOf:fix/ownership-lock-expiry-log-level
Sep 10, 2026
Merged

merlimat merged 1 commit into
apache:masterfrom
SongOf:fix/ownership-lock-expiry-log-level

Conversation

@SongOf

@SongOf SongOf commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Motivation

ResourceLockImpl.release() completes the lock's expiry future, so OwnershipCache's lock-expiry
listener also runs at the tail of every deliberate release: every bundle unload, the split cleanup,
and shutdown. The listener logs Resource lock has expired at INFO unconditionally, so a routine
unload emits a line that reads like a lost metadata session. Operators get one false hit per unload,
which buries the cases that line is meant to surface — a lost session or a failed revalidation.

Modifications

Both removeOwnership overloads take the lock out of locallyAcquiredLocks before calling
release(), so the lock is still registered when the listener runs only if it died on its own. The
INFO line is now gated on the two-arg locallyAcquiredLocks.remove returning true; a deliberate
release, or a stale listener whose generation has already been replaced, logs at DEBUG instead.
Nothing outside logging changes.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added OwnershipCacheTest.testReleasingOwnershipDoesNotReportTheLockAsExpired: a normal
    removeOwnership must not produce an INFO Resource lock has expired. Fails before the change.
  • Added OwnershipCacheTest.testLockDyingOutsideOfReleaseIsReportedAsExpired: a lock that dies
    without going through removeOwnership is still reported at INFO, so the line cannot be dropped
    instead of gated.
  • Both use the existing TestLogAppender. OwnershipCacheTest (20), NamespaceUnloadingTest,
    NamespaceOwnershipListenerTest and the unload / split cases of NamespaceServiceTest pass
    locally; ./gradlew quickCheck is clean.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. The conditional removal correctly keeps deliberate ownership releases quiet while retaining the expiry message for a lock that is still registered. The two regression tests cover both cases. No issues found.

@merlimat
merlimat merged commit 8482e16 into apache:master Sep 10, 2026
82 of 84 checks passed
@lhotari lhotari added this to the 5.0.0-M2 milestone Sep 10, 2026
lhotari pushed a commit that referenced this pull request Sep 10, 2026
…ired resource lock (#26533)

Co-authored-by: maxlisongsong <[email protected]>
(cherry picked from commit 8482e16)
lhotari pushed a commit that referenced this pull request Sep 11, 2026
…ired resource lock (#26533)

Co-authored-by: maxlisongsong <[email protected]>
(cherry picked from commit 8482e16)
Radiancebobo pushed a commit to Radiancebobo/pulsar that referenced this pull request Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants