You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2023/08/31 18:25:16 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #21105: [improve] [ml] compress individual ack info to make maintain more records
poorbarcode opened a new pull request, #21105:
URL: https://github.com/apache/pulsar/pull/21105
### Motivation
Broker serialize `individualDeletedMessages` as an array `(1:1..1:5000],(2:-1..3:5000],,(100:-1..100:5000]`, each ledger will have a separate record, even if multiple ledgers are contiguous.
We can compress the contiguous records to one, such as `(1:1...100:5000]`. This can make one entry maintain more data.
### Modifications
Compress the contiguous records to one to make one entry maintain more data.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: x
--
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] [improve] [ml] compress individual ack info to make maintain more records [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #21105: [improve] [ml] compress individual ack info to make maintain more records
URL: https://github.com/apache/pulsar/pull/21105
--
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
[GitHub] [pulsar] github-actions[bot] commented on pull request #21105: [improve] [ml] compress individual ack info to make maintain more records
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21105:
URL: https://github.com/apache/pulsar/pull/21105#issuecomment-1741920906
The pr had no activity for 30 days, mark with Stale label.
--
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] [improve] [ml] compress individual ack info to make maintain more records [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #21105:
URL: https://github.com/apache/pulsar/pull/21105#discussion_r1893535797
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2954,53 +2971,99 @@ private static List<StringProperty> buildStringPropertiesMap(Map<String, String>
return stringProperties;
}
- private List<MLDataFormats.MessageRange> buildIndividualDeletedMessageRanges() {
+ @VisibleForTesting
+ List<MLDataFormats.MessageRange> buildIndividualDeletedMessageRanges() {
Review Comment:
Instead of adding "VisibleForTesting" annotation here, please extract this logic to a separate class which is tested separately. There's a lot of complexity here and this would be very hard to maintain in the future in this form. Smaller classes and smaller methods are a general preference in maintainable 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
Re: [PR] [improve] [ml] compress individual ack info to make maintain more records [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #21105:
URL: https://github.com/apache/pulsar/pull/21105#issuecomment-6182282348
Hi @poorbarcode, thanks for the contribution. The review asked to extract the merging logic into a separate class with its own unit tests, and the PR was left unfinished after that. ManagedCursorImpl has changed a lot since, and master now persists up to 200000 unacked ranges by default with LZ4 compression of the cursor info, so this would need a fresh start. We're going through the backlog of open PRs (https://lists.apache.org/thread/cgy7w0qgnq8s7j0ykyzgrn3hj4nb67wf), so I'm closing this one. If you'd like to continue, feel free to reopen it rebased on current master.
--
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