Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
lhotari commented on PR #23759: URL: https://github.com/apache/pulsar/pull/23759#issuecomment-2603945077 @poorbarcode I have replied in https://lists.apache.org/thread/rhb4hbbrqw3238l0563n9nm6s57jrb00 . -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
lhotari commented on PR #23759: URL: https://github.com/apache/pulsar/pull/23759#issuecomment-2602983252 > Hi Lari > > Since there is no more arguments in the email list, I dismissed your change request @poorbarcode In Apache projects, we continue discussions until there's consensus on important decisions. We haven't reached consensus yet. Let's postpone cherry-picking to branch-4.0 until there's consensus. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode merged PR #23759: URL: https://github.com/apache/pulsar/pull/23759 -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode commented on PR #23759: URL: https://github.com/apache/pulsar/pull/23759#issuecomment-2601160424 Hi Lari Since there is no more arguments in the email list, I dismissed your change request -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
thetumbled commented on code in PR #23759: URL: https://github.com/apache/pulsar/pull/23759#discussion_r1914559497 ## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ## @@ -2243,6 +2243,10 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " will only be tracked in memory and messages will be redelivered in case of" + " crashes.") private int managedLedgerMaxUnackedRangesToPersist = 1; +@FieldContext( +category = CATEGORY_STORAGE_ML, +doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate") +private boolean managedLedgerPersistIndividualAckAsLongArray = false; Review Comment: I support that keeping compatibility should comes first, which impact most of the users. and i wonder whether we can fix the issue without introducing the switch to disable the improvement introduced by #9292. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
thetumbled commented on code in PR #23759: URL: https://github.com/apache/pulsar/pull/23759#discussion_r1914559497 ## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ## @@ -2243,6 +2243,10 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " will only be tracked in memory and messages will be redelivered in case of" + " crashes.") private int managedLedgerMaxUnackedRangesToPersist = 1; +@FieldContext( +category = CATEGORY_STORAGE_ML, +doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate") +private boolean managedLedgerPersistIndividualAckAsLongArray = false; Review Comment: I support that keeping compatibility should comes first, which impact most of the users. and i wonder whether we can fix the issue without introducing the switch to disable the improvement introduced by #9292. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode commented on PR #23759: URL: https://github.com/apache/pulsar/pull/23759#issuecomment-2589375269 @lhotari Answered your question in the discussion channel: https://lists.apache.org/thread/kfm7n6xf9pf7h76k4gx83zv3c2kj6yy1 -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
lhotari commented on code in PR #23759: URL: https://github.com/apache/pulsar/pull/23759#discussion_r1914391058 ## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ## @@ -2243,6 +2243,10 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " will only be tracked in memory and messages will be redelivered in case of" + " crashes.") private int managedLedgerMaxUnackedRangesToPersist = 1; +@FieldContext( +category = CATEGORY_STORAGE_ML, +doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate") +private boolean managedLedgerPersistIndividualAckAsLongArray = false; Review Comment: Mailing list thread: https://lists.apache.org/thread/kfm7n6xf9pf7h76k4gx83zv3c2kj6yy1 When we say there's support for downgrading (rollback), it doesn't mean users don't need to take any action. We ensure downgrade compatibility by providing an upgrade guide that explains how to configure the system to allow rollbacks without losing state information. If we didn't do this, we'd always be stuck in the same situation whenever a new LTS version comes out. Let's start making progress: - keep PR 9292 as default - implement PR 23759 without changing the PR 9292 default - write a proper upgrade guide for Pulsar 4.0 where rollback considerations are explained -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
eolivelli commented on code in PR #23759: URL: https://github.com/apache/pulsar/pull/23759#discussion_r1913331565 ## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ## @@ -2243,6 +2243,10 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " will only be tracked in memory and messages will be redelivered in case of" + " crashes.") private int managedLedgerMaxUnackedRangesToPersist = 1; +@FieldContext( +category = CATEGORY_STORAGE_ML, +doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate") +private boolean managedLedgerPersistIndividualAckAsLongArray = false; Review Comment: As written on the mailing list I think that we cannot break compatibility with Pulsar 4.0.0 and 4.0.1, it is "worse" than breaking compatibility with "3.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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode commented on code in PR #23759: URL: https://github.com/apache/pulsar/pull/23759#discussion_r1899239478 ## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ## @@ -2243,6 +2243,10 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " will only be tracked in memory and messages will be redelivered in case of" + " crashes.") private int managedLedgerMaxUnackedRangesToPersist = 1; +@FieldContext( +category = CATEGORY_STORAGE_ML, +doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate") +private boolean managedLedgerPersistIndividualAckAsLongArray = false; Review Comment: Yes, will rollback the default behavior to `3.0.x`, in other words, removed the default behavior changes that introduced by #9292 -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
lhotari commented on code in PR #23759: URL: https://github.com/apache/pulsar/pull/23759#discussion_r1899189094 ## pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java: ## @@ -2243,6 +2243,10 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece + " will only be tracked in memory and messages will be redelivered in case of" + " crashes.") private int managedLedgerMaxUnackedRangesToPersist = 1; +@FieldContext( +category = CATEGORY_STORAGE_ML, +doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate") +private boolean managedLedgerPersistIndividualAckAsLongArray = false; Review Comment: Does change the existing Pulsar 4.0 behavior when this defaults to `false`? -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode commented on code in PR #23759:
URL: https://github.com/apache/pulsar/pull/23759#discussion_r1898179996
##
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##
@@ -2367,7 +2383,14 @@ public void asyncDelete(Iterable positions,
AsyncCallbacks.DeleteCallb
}
// Add a range (prev, pos] to the set. Adding the previous
entry as an open limit to the range will
// make the RangeSet recognize the "continuity" between
adjacent Positions.
-Position previousPosition =
ledger.getPreviousPosition(position);
+// Before https://github.com/apache/pulsar/pull/21105 is
merged, the range does not support crossing
+// multi ledgers, so the first position's entryId maybe
"-1".
+Position previousPosition;
+if (position.getEntryId() == 0) {
+previousPosition =
PositionFactory.create(position.getLedgerId(), -1);
+} else {
+previousPosition =
ledger.getPreviousPosition(position);
+}
Review Comment:
> It's better to move to the internal of the getPreviousPosition() method.
`getPreviousPosition` returns a meaningful position, in other words, it
returns a position that exactly exists. And other methods rely on this feature,
such as
https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L3621-L3623.
Only cursor individual ack range does not allow crossing ledgers so far.
```java
if (specifiedLac.getEntryId() < 0) {
// Calculate the meaningful LAC.
Position actLac = getPreviousPosition(specifiedLac);
...
}
```
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode commented on code in PR #23759:
URL: https://github.com/apache/pulsar/pull/23759#discussion_r1898179996
##
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##
@@ -2367,7 +2383,14 @@ public void asyncDelete(Iterable positions,
AsyncCallbacks.DeleteCallb
}
// Add a range (prev, pos] to the set. Adding the previous
entry as an open limit to the range will
// make the RangeSet recognize the "continuity" between
adjacent Positions.
-Position previousPosition =
ledger.getPreviousPosition(position);
+// Before https://github.com/apache/pulsar/pull/21105 is
merged, the range does not support crossing
+// multi ledgers, so the first position's entryId maybe
"-1".
+Position previousPosition;
+if (position.getEntryId() == 0) {
+previousPosition =
PositionFactory.create(position.getLedgerId(), -1);
+} else {
+previousPosition =
ledger.getPreviousPosition(position);
+}
Review Comment:
> It's better to move to the internal of the getPreviousPosition() method.
`getPreviousPosition` returns a meaningful position, in other words, it
returns a position that exactly exists. And other methods rely on this feature,
such as
https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L3621-L3623
```java
if (specifiedLac.getEntryId() < 0) {
// Calculate the meaningful LAC.
Position actLac = getPreviousPosition(specifiedLac);
...
}
```
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
codelipenghui commented on code in PR #23759:
URL: https://github.com/apache/pulsar/pull/23759#discussion_r1898079573
##
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##
@@ -2367,7 +2383,14 @@ public void asyncDelete(Iterable positions,
AsyncCallbacks.DeleteCallb
}
// Add a range (prev, pos] to the set. Adding the previous
entry as an open limit to the range will
// make the RangeSet recognize the "continuity" between
adjacent Positions.
-Position previousPosition =
ledger.getPreviousPosition(position);
+// Before https://github.com/apache/pulsar/pull/21105 is
merged, the range does not support crossing
+// multi ledgers, so the first position's entryId maybe
"-1".
+Position previousPosition;
+if (position.getEntryId() == 0) {
+previousPosition =
PositionFactory.create(position.getLedgerId(), -1);
+} else {
+previousPosition =
ledger.getPreviousPosition(position);
+}
Review Comment:
It's better to move to the internal of the getPreviousPosition() method.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
codecov-commenter commented on PR #23759: URL: https://github.com/apache/pulsar/pull/23759#issuecomment-2554470616 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23759?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `40.0%` with `15 lines` in your changes missing coverage. Please review. > Project coverage is 31.49%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e5e3873`)](https://app.codecov.io/gh/apache/pulsar/commit/e5e3873a36d5de0dbd1118e5eccbb61f4eb52895?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 801 commits behind head on master. | [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23759?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...che/bookkeeper/mledger/impl/ManagedCursorImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23759?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FManagedCursorImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JJbXBsLmphdmE=) | 26.31% | [13 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23759?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...pache/bookkeeper/mledger/impl/RangeSetWrapper.java](https://app.codecov.io/gh/apache/pulsar/pull/23759?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FRangeSetWrapper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL1JhbmdlU2V0V3JhcHBlci5qYXZh) | 83.33% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23759?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | > :exclamation: There is a different number of reports uploaded between BASE (bbc6224) and HEAD (e5e3873). Click for more details. > > HEAD has 2 uploads less than BASE > >| Flag | BASE (bbc6224) | HEAD (e5e3873) | >|--|--|--| >|unittests|2|0| > Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23759?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) ```diff @@ Coverage Diff @@ ## master #23759 +/- ## = - Coverage 73.57% 31.49% -42.09% + Complexity32624 86-32538 = Files 1877 1697 -180 Lines139502 136260 -3242 Branches 1529915437 +138 = - Hits 10263842912-59726 - Misses2890886789+57881 + Partials 7956 6559 -1397 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23759/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23759/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.68% <40.00%> (+2.09%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23759/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.67% <40.00%> (-0.66%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23759/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | Flags with carried forward coverage won't be shown. [Click he
Re: [PR] [fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled [pulsar]
poorbarcode commented on PR #23759: URL: https://github.com/apache/pulsar/pull/23759#issuecomment-2553348466 /pulsarbot rerun-failure-checks -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
