lhotari commented on code in PR #25873: URL: https://github.com/apache/pulsar/pull/25873#discussion_r3419970161
########## pip/pip-480.md: ########## @@ -0,0 +1,153 @@ +# PIP-480: Add cursorless readEntries API to ManagedLedger + +# Background knowledge + +Most `ManagedLedger` reads go through `ManagedCursor`. +A cursor is durable state: read position, mark-delete position, and individual deleted entries. +That is the right model for Pulsar subscriptions because the broker owns subscription progress and uses cursor state for +acknowledgement, backlog accounting, and ledger retention. + +Protocol handlers such as KoP do not always want that state. Kafka offsets are maintained outside Pulsar's +`ManagedCursor` abstraction. If KoP opens a cursor only to fetch entries, it creates extra managed-ledger metadata and +then has to keep the cursor lifecycle aligned with Kafka offsets. + +`ManagedLedger` already exposes `asyncReadEntry(Position, ...)` for reading a single entry by position, but it does not +provide a batch read primitive for this use case. Callers have to repeat the ledger traversal logic themselves: validate +the start position, cross ledger boundaries, skip empty ledgers, and stop at the current last confirmed entry. + +# Motivation + +Provide a cursorless read path for downstream projects like KoP. Review Comment: There's currently a reason to have a cursor. For example, a cursor will prevent trimming while the cursor is active. Another important reason for the cursor is the managed ledger cache (a.k.a "broker cache"). It is possible to implement the ManagedLedger internally in a way where an external cursor wouldn't be required and it would be handled internally. However, that breaks the original abstraction which ManagedLedger & ManagedCursor provide. > If KoP opens a cursor only to fetch entries, it creates extra managed-ledger metadata and then has to keep the cursor lifecycle aligned with Kafka offsets. For non-durable cursors "extra metadata" is cheap and only in memory. Does KoP use non-durable cursors? > `ManagedLedger` already exposes `asyncReadEntry(Position, ...)` for reading a single entry by position, but it does not provide a batch read primitive for this use case. From an abstraction perspective, this is a mistake and already breaks the abstract. I added the `asyncReadEntry` to `ManagedLedger` interface in #23311 since Pulsar already depended directly on `ManagedLedgerImpl` having this method. (#23311 was about having the possibility to implement a 3rd party `ManagedLedger` implementation such as SN Ursa). However, making the abstraction worse isn't a great approach. For the batch read, there's also another problem that this introduces when there's no "waiting mode". Without a waiting mode, there would be unnecessary polling when cursors are consuming the tail. One of the existing problems in the ManagedCursor API is that there's no streaming API. It would be more useful to start a "reactive" streaming read with a possibility to cancel. I believe that such an API would be useful for batching reads. With a batching read, there could be a batch deadline when the cursor is in tailing mode. This would resolve unnecessary tight loops with tailing reads. This challenge exists also for moving to batch reads from bookkeeper for Pulsar. Instead of waiting for the requested number of entries, the read from ManagedCursor should support returning less entries when no more are available. These were the thoughts based on some previous experiences of the bookkeeper batch read and integrating it efficiently to Pulsar. I didn't check all details. In summary: I believe that the correct path would be to ensure that the ManagedLedger abstraction is involved in a reasonable direction. One possible solution would be to have a separate interface for random reads which could be created by calling a method in ManagedLedger. We could also migrate the existing `asyncReadEntry` entry to that and also add a similar batch read method as long as the details about deadlines for batching of tailing reads is addressed so that the usage doesn't result in inefficiencies. Could you revisit the proposal in that direction? (a separate random reads interface, which also documents the abstraction and the various requirements for implementations if such exist (for example related to retention/trimming while a read is performed)) -- 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]
