Skip to content

[fix][metadata] Record get op stats on the correct completion branch in AbstractMetadataStore#26201

Open
SongOf wants to merge 1 commit into
apache:masterfrom
SongOf:fix/metadata-get-op-stats
Open

[fix][metadata] Record get op stats on the correct completion branch in AbstractMetadataStore#26201
SongOf wants to merge 1 commit into
apache:masterfrom
SongOf:fix/metadata-get-op-stats

Conversation

@SongOf

@SongOf SongOf commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Motivation

Since #24580, the whenComplete callback in AbstractMetadataStore#get has its
branch contents inverted. The failure branch (t != null) calls
v.ifPresent(...), but on exceptional completion v is always null, so the
callback throws a NullPointerException that short-circuits
metadataStoreStats.recordGetOpsFailed. Per whenComplete semantics the NPE is
only attached to the original exception as a suppressed exception, so callers
still see the original failure — which is why this went unnoticed. The actual
damage is to observability:

  • Failed get operations are never recorded in the
    pulsar_metadata_store_ops_latency{type="get", status="fail"} histogram, so
    the metric silently reports zero failures even when the metadata store is
    unhealthy (the only path still counted is the synchronous invalid-path check).
  • nodeSizeStats.recordGetRes — which [fix][broker]Fix never recovered metadata store bad version issue if received a large response from ZK #24580 intended to run on the success
    path (cf. recordGetChildrenRes in getChildren) — is never invoked at all,
    so node-size stats for get responses were never collected.

Modifications

  • In AbstractMetadataStore#get, move recordGetOpsFailed back to being the
    sole action of the failure branch, and move
    v.ifPresent(... nodeSizeStats.recordGetRes ...) into the success branch
    where it was intended to run.
  • Add AbstractMetadataStoreGetStatsTest covering both halves of the bug
    against a minimal AbstractMetadataStore test double.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added AbstractMetadataStoreGetStatsTest#testGetFailureRecordsFailedOpAndPropagatesOriginalException:
    injects a failing storeGet and asserts the original exception propagates
    without a suppressed NPE and that the status="fail" histogram count
    increments — fails before this fix.
  • Added AbstractMetadataStoreGetStatsTest#testGetSuccessRecordsNodeSizeStats:
    asserts MetadataNodeSizeStats#recordGetRes is invoked with the returned
    GetResult and the status="success" count increments — fails before this
    fix (recordGetRes was never called).

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

The existing pulsar_metadata_store_ops_latency{type="get", status="fail"}
histogram starts reporting real values (it previously never recorded async get
failures), and node-size stats for get responses are now actually collected. No
metric names, labels, or types change.

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