[improve][broker] Clean up config changes without a PIP since the 4.0 LTS #24027
Conversation
… LTS - Remove `managedLedgerOffloadReadThreads` from apache#24025 - Remove `managedLedgerMaxReadsInFlightPermitsAcquireTimeoutMillis` and `managedLedgerMaxReadsInFlightPermitsAcquireQueueSize` from apache#23901 - Remove `managedLedgerCursorResetLedgerCloseTimestampMaxClockSkewMillis` from apache#22792 The configs above only increase the complexity and are hard to configure. Add more comments to `managedLedgerPersistIndividualAckAsLongArray` from apache#23759. This config was added to keep the compatibility from 3.x or earlier so it has value to retain. 3.x users must configure it with false when upgrading to 4.0.
BewareMyPower
requested review from
lhotari,
hangc0276,
Technoboy-,
dao-jun,
zymap,
poorbarcode and
thetumbled
There was a problem hiding this comment.
For some of the added configuration you are right that PIPs are needed.
Most configurables are settings for internal implementation details that don't need to be changed. In many cases, it will be useful if the value can be modified if a user faces an issue that could be solved by tuning the limit.
For example, #23901 introduced managedLedgerMaxReadsInFlightPermitsAcquireTimeoutMillis and managedLedgerMaxReadsInFlightPermitsAcquireQueueSize. The original feature contained a gap where the solution would never work properly when the system was under heavy load. That's the whole point of the managedLedgerMaxReadsInFlightSizeInMB solution that it adds a limit. If the limit doesn't work, that's a bug. It's not a new feature when that problem is fixed.
What is the problem that these new configs have caused to our users?
We can continue this discussion on the mailing list.
Just a simple question. When should users modify this config? |
| # When it's false, the behavior will be the same with 3.x or earlier | ||
| # Modifying this config could lose existing individual acknowledgments, so you should configure it with false when | ||
| # upgrading from 3.x to 4.0 or later if you don't want to lose these acknowledgments. | ||
| managedLedgerPersistIndividualAckAsLongArray=true |
There was a problem hiding this comment.
There is a conclusion that we will never set the default value of managedLedgerPersistIndividualAckAsLongArray to true for branch-4.0.x
The key point is, since there are too many configurations, it's impossible for users to tune configs one by one. Back to the example of #23901, how can users realize the issue is related to the asynchronous InflightReadsLimiter? |
lhotari
added
release/3.3.7
release/4.0.5
and removed
release/3.3.6
release/4.0.4
labels
managedLedgerOffloadReadThreadsfrom [improve] [broker] Separate offload read and write thread pool #24025managedLedgerMaxReadsInFlightPermitsAcquireTimeoutMillisandmanagedLedgerMaxReadsInFlightPermitsAcquireQueueSizefrom [fix][broker] Make InflightReadsLimiter asynchronous and apply it for replay queue reads #23901managedLedgerCursorResetLedgerCloseTimestampMaxClockSkewMillisfrom [improve][broker] Optimize subscription seek (cursor reset) by timestamp #22792 The configs above only increase the complexity and are hard to configure.Add more comments to
managedLedgerPersistIndividualAckAsLongArrayfrom #23759. This config was added to keep the compatibility from 3.x or earlier so it has value to retain. 3.x users must configure it with false when upgrading to 4.0.Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: