You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "merlimat (via GitHub)" <gi...@apache.org> on 2026/10/13 19:05:50 UTC
[PR] [fix][meta] Fix the upper bound of the Oxia secondary-index scans [pulsar]
merlimat opened a new pull request, #26890:
URL: https://github.com/apache/pulsar/pull/26890
### Motivation
With Oxia as the metadata store, the PIP-473 transaction metadata of scalable topics mixes up the records of
segments, subscriptions and transactions whose names extend one another. `TxnMetadataStore` finds the `/txn/op`
records of a segment, of a `(segment, subscription)` and of a transaction with exact-key `scanByIndex` lookups. On
Oxia, each lookup also returns the records whose key extends the one looked up:
- `listWritesBySegment(".../0000-7fff-1")` also returns the writes on `.../0000-7fff-12` (segment descriptors are
`%04x-%04x-%d`);
- `listAcksBySegmentSubscription(segment, "sub")` also returns the acks of `sub2`, `sub-1`, ...;
- `listOpsByTxn("1_5")` also returns the ops of `1_50`, `1_51`, ...
As a result:
- `MetadataPendingAckStore` recovery adds other subscriptions' transactions to its open set. Once they end,
`deleteAckOpsForSegmentSubscriptionAndTxn`, which only checks the transaction id, deletes their ack records on
the other subscription.
- `MetadataTransactionBuffer` recovery can pick up an open transaction that only wrote to a segment whose id
extends this one. It holds `maxReadPosition` at the watermark until an event of this segment makes it re-read
the transaction's header, and the coordinator never publishes one for that transaction. Applying its outcome
then deletes its write records on the other segment, which that segment's buffer still needs if it hasn't
applied the outcome yet.
- `TransactionCoordinatorV5` publishes a decision to the participants of other transactions, and `gcOneTxn` keeps
a header while another transaction's ops exist, publishing the decision again on every GC pass.
The cause is in `OxiaMetadataStore.storeScanByIndex`. `scanByIndex` takes inclusive bounds, and the method maps them
onto Oxia's half-open `list(start, end, UseIndex(...))` by ending the range at `<prefix>/<toKey>~`. Oxia compares
the bounds with whole index entries: the secondary key, a `0x01` separator and the escaped primary key, in the
namespace's key order, which is hierarchical by default. So the range also covers every key that extends `toKey`
with a character below `~`.
Without `toKey`, the method ends the range at `<prefix>0`. That bound has one `/` fewer than the entries, so the
hierarchical sorting puts it before all of them, and the scan returns nothing; no caller does this today.
`ScalableTopicResources.findScalableTopicsByPropertiesAsync` re-checks its predicate, so for it the over-match only
costs extra reads.
### Example
With an Oxia metadata store (`oxia/oxia:nightly-beta`), on master:
```java
// seg1 = segment://public/default/topic/0000-7fff-1, seg12 = .../0000-7fff-12
txn.appendOp("1_1", new TxnOp(WRITE, seg1, null, 1, 1, null));
txn.appendOp("1_1", new TxnOp(WRITE, seg12, null, 2, 1, null));
txn.appendOp("1_12", new TxnOp(WRITE, seg12, null, 2, 2, null));
txn.appendOp("1_1", new TxnOp(ACK, seg1, "sub", 3, 1, false));
txn.appendOp("1_1", new TxnOp(ACK, seg1, "sub2", 3, 2, false));
txn.appendOp("1_12", new TxnOp(ACK, seg1, "sub", 3, 3, false));
txn.listWritesBySegment(seg1, c); // 1:1, 2:1, 2:2 (expected 1:1)
txn.listAcksBySegmentSubscription(seg1, "sub", c); // 3:1, 3:2, 3:3 (expected 3:1, 3:3)
txn.listOpsByTxn("1_1", c); // also 1_12's 2:2 and 3:3
txn.deleteWriteOpsForSegmentAndTxn(seg1, "1_1");
txn.deleteAckOpsForSegmentSubscriptionAndTxn(seg1, "sub", "1_1");
// 1_1's write on seg12 (2:1) and its ack on sub2 (3:2) are deleted too
```
The bounds today:
```java
String scopedTo = toKeyInclusive == null
? scanPathPrefix + "0" // '0' (0x30) is the lex successor of '/' (0x2F)
: scanPathPrefix + "/" + toKeyInclusive + "~";
```
Listing the index with the Oxia client directly gives the same results on `oxia/oxia:nightly-beta`, `0.16.11` and an
August build of Oxia's main branch:
| Range (index keys under `P/`) | Returned |
|---|---|
| `[P/k-1, P/k-1~)` | `k-1`, `k-1%2F`, `k-1*`, `k-1+`, `k-1.x`, `k-12`, `k-1:a`, `k-1_` |
| `[P/k-1, P/k-1` + `0x02)` | `k-1` |
| `[P/k-1, P/k-1` + `0x00)` | nothing |
| `[P/, P0)` | nothing |
| `[P/, P//)` | every key of the level |
### Modifications
```java
String scopedTo = toKeyInclusive == null
? scanPathPrefix + "//"
: scanPathPrefix + "/" + toKeyInclusive + "\u0002";
```
`toKey` followed by `0x02` sorts after every entry of `toKey` (`toKey`, `0x01`, the escaped primary key), and before
the entries of every key that extends `toKey` with a character above `0x02`, in both key sortings. The transaction
index keys are URL-encoded names and transaction ids, which never contain lower characters. Without `toKey`, the
range ends at the end of the records' level, `<prefix>//`. `getChildrenFromStore` and `storeScanChildren` use the
same convention for the hierarchical sorting.
A comment documents the dependency on Oxia's index entry layout, which hasn't changed since Oxia added secondary
indexes. The cross-backend tests run against `oxia/oxia:nightly-beta`, so a change there would show up. The store
can't check the secondary key of each record instead: Oxia's server returns it with index range-scan results since
oxia-db/oxia#1508, but the Java client's `GetResult` doesn't expose it yet.
`TxnMetadataStore` needs no change: it already asks for `[key, key]`, and the stores without a native index apply
its exact predicates.
### Verifying this change
- `MetadataStoreSecondaryIndexTest`: 3 new tests on every provider (ZooKeeper, Memory, RocksDB, Oxia,
MockZooKeeper): a point lookup and an inclusive range don't return the keys that extend their end, and a scan
without `toKey` returns the rest of the keys. Before the change, only the Oxia cases fail, since the fallback scan
applies the filter. All 60 tests of the class pass with it.
- `MetadataCacheSecondaryIndexTest`, `OxiaPartitionKeyTest` and `ScalableTopicPropertyIndexTest` pass.
- On a real Oxia (Testcontainers, `oxia/oxia:nightly-beta`), the `TxnMetadataStore` example above returns only the
expected records, and the deletes keep `1_1`'s records on `seg12` and `sub2`.
### 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
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][meta] Fix the upper bound of the Oxia secondary-index scans [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #26890:
URL: https://github.com/apache/pulsar/pull/26890#discussion_r4263540025
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/oxia/OxiaMetadataStore.java:
##########
@@ -256,18 +256,19 @@ protected CompletableFuture<Void> storeScanByIndex(
ScanConsumer consumer, Set<Option> opts) {
// Index entries are stored at "<parentPath>/<secondaryKey>" by doStorePut, where
// parentPath == scanPathPrefix for records directly under the prefix. The API uses
- // inclusive-on-both-sides bounds; Oxia's list(start, end, ...) is half-open, so we
- // append a sentinel character to the upper bound to widen the half-open range to
- // include `toKeyInclusive`. We use '~' (0x7E) — it lex-sorts after every printable
- // ASCII byte, which is a safe choice for the secondary keys callers actually use today
- // (numeric-encoded timestamps, fixed-tag enums, etc.). Callers that need full byte-range
- // coverage can pass `toKeyInclusive=null` or use a different scheme.
+ // inclusive-on-both-sides bounds; Oxia's list(start, end, ...) is half-open, and it compares
+ // the bounds with whole index entries, which Oxia stores as the secondary key, a 0x01
+ // separator and the escaped primary key. So the end that follows the entries of
+ // `toKeyInclusive`, and precedes those of the keys that extend it (e.g. "k-12" for "k-1"),
+ // is `toKeyInclusive` followed by 0x02. Without `toKeyInclusive`, the range ends at the end
+ // of the level of the records, as the range [path + "/", path + "//") of
+ // getChildrenFromStore does with Oxia's hierarchical key sorting.
String scopedFrom = fromKeyInclusive == null
? scanPathPrefix + "/"
: scanPathPrefix + "/" + fromKeyInclusive;
String scopedTo = toKeyInclusive == null
- ? scanPathPrefix + "0" // '0' (0x30) is the lex successor of '/' (0x2F)
- : scanPathPrefix + "/" + toKeyInclusive + "~";
+ ? scanPathPrefix + "//"
Review Comment:
Non-blocking: this unbounded endpoint still assumes slash-free secondary keys. On real Oxia, a direct child indexed by `tenant/ns1` is found by an exact lookup but omitted with a null upper bound. I reproduced this with both `[null, null]` and `["tenant/ns1", null]`, before and after the PR, so it need not block the transaction fix. Could we document the limitation and track regression coverage separately?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][meta] Fix the upper bound of the Oxia secondary-index scans [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #26890:
URL: https://github.com/apache/pulsar/pull/26890
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][meta] Fix the upper bound of the Oxia secondary-index scans [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #26890:
URL: https://github.com/apache/pulsar/pull/26890#discussion_r4264900538
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/oxia/OxiaMetadataStore.java:
##########
@@ -256,18 +256,19 @@ protected CompletableFuture<Void> storeScanByIndex(
ScanConsumer consumer, Set<Option> opts) {
// Index entries are stored at "<parentPath>/<secondaryKey>" by doStorePut, where
// parentPath == scanPathPrefix for records directly under the prefix. The API uses
- // inclusive-on-both-sides bounds; Oxia's list(start, end, ...) is half-open, so we
- // append a sentinel character to the upper bound to widen the half-open range to
- // include `toKeyInclusive`. We use '~' (0x7E) — it lex-sorts after every printable
- // ASCII byte, which is a safe choice for the secondary keys callers actually use today
- // (numeric-encoded timestamps, fixed-tag enums, etc.). Callers that need full byte-range
- // coverage can pass `toKeyInclusive=null` or use a different scheme.
+ // inclusive-on-both-sides bounds; Oxia's list(start, end, ...) is half-open, and it compares
+ // the bounds with whole index entries, which Oxia stores as the secondary key, a 0x01
+ // separator and the escaped primary key. So the end that follows the entries of
+ // `toKeyInclusive`, and precedes those of the keys that extend it (e.g. "k-12" for "k-1"),
+ // is `toKeyInclusive` followed by 0x02. Without `toKeyInclusive`, the range ends at the end
+ // of the level of the records, as the range [path + "/", path + "//") of
+ // getChildrenFromStore does with Oxia's hierarchical key sorting.
String scopedFrom = fromKeyInclusive == null
? scanPathPrefix + "/"
: scanPathPrefix + "/" + fromKeyInclusive;
String scopedTo = toKeyInclusive == null
- ? scanPathPrefix + "0" // '0' (0x30) is the lex successor of '/' (0x2F)
- : scanPathPrefix + "/" + toKeyInclusive + "~";
+ ? scanPathPrefix + "//"
Review Comment:
Documented in e801a10bb5: the comment above the bounds now says the ranges follow Oxia's hierarchical sorting, so a scan without an upper bound leaves out the keys with a '/' (and `["b", "tenant/ns1"]` also returns `"zz"`); coverage for keys with a '/' is left for a follow-up.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org