lhotari commented on PR #25754:
URL: https://github.com/apache/pulsar/pull/25754#issuecomment-4434971082
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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
