Skip to content

[improve][broker] PIP-473: transaction metadata data layer#25754

Merged
merlimat merged 5 commits into
apache:masterfrom
merlimat:mmerli/pip-473-data-layer
May 12, 2026
Merged

[improve][broker] PIP-473: transaction metadata data layer#25754
merlimat merged 5 commits into
apache:masterfrom
merlimat:mmerli/pip-473-data-layer

Conversation

@merlimat
Copy link
Copy Markdown
Contributor

Summary

Foundation data layer for PIP-473 (metadata-driven transactions). Introduces typed records + path/index constants + a TxnMetadataStore façade over MetadataStore, ready for the v5 TC to plug into in a follow-up.

  • New records under pulsar-broker/.../transaction/metadata/:
    • TxnHeader — header at /txn/<txnId>, the CAS linearization point for endTxn
    • TxnOp — op-log entry written under /txn-op/<txnId> with SequenceKeysDeltas
    • TxnEvent — per-segment / per-(segment, sub) notification event
    • Versioned<T>(value, version) pair returned from reads so callers can CAS
    • TxnState, TxnOpKind — Jackson-friendly enums (serialized by name)
  • TxnPaths — path templates, index names, key formatters
  • TxnMetadataStore façade — header CRUD with CAS, op append with kind-specific secondary indexes, deadline / final-state range queries (via scanByIndex), and segment / subscription event publish + subscribe (via subscribeSequence)
  • Bundled metadata-layer fix: the __seq_counter__ sidecar that backs SequenceKeysDeltas on non-native stores was leaking through scanChildren and the scanByIndex fallback. Filter it out in AbstractMetadataStore's defaults plus the Memory and RocksDB native overrides. Oxia uses native sequence keys (no sidecar) and is unaffected.

No runtime path consumes this yet — the v5 TC PR will wire it up.

Test plan

  • pulsar-broker:test --tests TxnMetadataStoreTest — 7 tests on Memory: header CAS lifecycle, write/ack op append + index scan, deadline range scan (filters terminal + out-of-range), final-state range scan, segment + subscription event publish + subscribe
  • pulsar-metadata:test --tests MetadataStoreScanChildrenTest — added skipsSequenceCounterSidecars across ZooKeeper / Memory / RocksDB / Oxia / MockZooKeeper
  • pulsar-metadata:test --tests SequenceKeysTest + MetadataStoreSecondaryIndexTest — pre-existing suites still green after the abstract-layer change
  • Checkstyle clean on pulsar-broker + pulsar-metadata

merlimat added 3 commits May 12, 2026 12:55
Introduce schema-versioned JSON records (TxnHeader, TxnOp, TxnEvent,
Versioned), path/index constants (TxnPaths), and a typed TxnMetadataStore
façade over MetadataStore covering: header CAS, op-log append with kind-
specific secondary indexes, deadline / final-state range queries, and
per-segment and per-(segment, subscription) event publish + subscribe via
SequenceKeysDeltas + subscribeSequence.

Bundled metadata-layer fix: the synthesized "__seq_counter__" sidecar that
backs SequenceKeysDeltas on non-native stores was leaking through
scanChildren and the scanByIndex fallback. Filter it out in the abstract
fallback plus the Memory and RocksDB native overrides (Oxia is unaffected —
its sequence keys are native and have no sidecar). Regression test covers
all five backends.
- Drop @JsonInclude(NON_NULL) from records — already in the global
  ObjectMapperFactory config.
- Drop premature schema versioning (version field + CURRENT_VERSION). Will
  be added back if/when an incompatible change is required.
- Replace TxnState and TxnOpKind constant classes with enums; Jackson
  serializes them by name so the wire format is unchanged.
- TxnHeader: use Duration for timeout and Instant for createdAt / finalizedAt
  so units are obvious at the type level.
- TxnMetadataStore: don't cache the ObjectMapper in a static — go through
  ObjectMapperFactory.getMapper() on each call so any future thread-local
  routing works as intended.
@lhotari
Copy link
Copy Markdown
Member

lhotari commented May 12, 2026

Claude Code review findings to check:

  1. [INTENT MISMATCH] Raw segment URIs are embedded into MetadataStore paths without encoding — TxnPaths.segmentEventsParent / subscriptionEventsParent in
  pulsar-broker/.../transaction/metadata/TxnPaths.java:90-102
    - PIP-473 segment names take the form segment://tenant/ns/topic/<hexStart>-<hexEnd>-<segmentId> (see existing comment at MLPendingAckStore.java:534). Concatenated into a metadata path,
  that produces e.g. /txn-segment-events/segment://tenant/ns/topic/... — which contains // (empty path component) and :.
    - ZooKeeper's PathUtils.validatePath rejects this; Oxia, while not slash-validating, would interpret the embedded / as path levels and break the sidecar / sequence-key semantics.
    - The existing convention in transaction code (MLPendingAckStore.java:533, TopicTransactionBuffer.java:868) is Codec.encode(...). This data layer breaks that convention without saying so.
    - Recommended: Either encode segment (and subscription) in TxnPaths helpers, or add a @throws/precondition comment documenting that callers must pre-encode. Currently the tests use
  "segment://A" only because Memory happens to tolerate it — that's a misleading green light for the v5 TC PR.
  2. [BUG] isSequenceCounterChild filter uses a permissive endsWith predicate — pulsar-metadata/.../AbstractMetadataStore.java:608-612, 651-655, 776-782
    - A user record whose final path segment ends with the literal __seq_counter__ would be silently filtered from scanChildren / scanByIndex. Low likelihood in practice, but easy to harden.
    - Since sequenceCounterPath(prefix) = prefix + "__seq_counter__" is always a sibling of the prefix at the same parent level, the sidecar name is always <prefix-leaf>__seq_counter__. A
  stricter check would require the sidecar to be co-located with a known prefix, or at minimum scope by an internal marker char not allowed in user keys.
    - Acceptable as-is if you're confident no user key will collide; worth a one-line comment justifying the looseness.

Copy link
Copy Markdown
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, just check the local Claude Code review findings I added a comment

merlimat added 2 commits May 12, 2026 15:20
…car filter

Two findings from Claude Code review (via @lhotari):

1. TxnPaths embedded raw segment URIs (which contain "://" and "/") directly
   into metadata-store paths. ZooKeeper's PathUtils.validatePath rejects that
   shape, and concatenating segment + sub into a composite key with ":" as
   the separator was ambiguous because segment names themselves contain ":".
   Fix: URL-encode the segment and subscription components everywhere they
   appear as a path component, partition key, or index value — same
   convention as MLPendingAckStore and TopicTransactionBuffer. TxnOp /
   TxnEvent JSON payloads keep the raw form (they're data, not keys).
   Test segment names switched to realistic "segment://tenant/ns/topic/..."
   so we actually exercise the encoding.

2. isSequenceCounterChild used a permissive String.endsWith match — a user
   record whose final path segment ends in the literal "__seq_counter__"
   would be silently filtered. Documenting that this is intentional: the
   suffix is a 16-character internal marker, the cost of a false positive
   is dropping that record from scanChildren/scanByIndex (no data loss),
   and a strict check would require either tracking active prefixes or
   reserving a delimiter that path backends forbid.
@merlimat merlimat merged commit e90a7c5 into apache:master May 12, 2026
43 checks passed
@merlimat merlimat deleted the mmerli/pip-473-data-layer branch May 12, 2026 23:49
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