-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Managed Ledger] Fix the incorrect total size when BrokerEntryMetadata is enabled #12714
[Managed Ledger] Fix the incorrect total size when BrokerEntryMetadata is enabled #12714
Conversation
Can we update the dataLength when calling setData()? We might get different value by readableBytes() which there is a reader read the data from the buffer. |
+1 |
@codelipenghui @eolivelli done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
…a is enabled (apache#12714) ### Motivation When the BrokerEntryMetadata is enabled, the total size in `ManagedLedgerImpl` is inaccurate. Because when the total size is updated in `OpAddEntry#safeRun`, the `dataLength` is the initial size of `data` when `OpAddEntry` is constructed, but `data` could be changed via `setData` method. The inaccurate total size could affect the retention size validation. Because in `ManagedLedgerImpl#internalTrimLedgers`, the total size reduces by the size of `LedgerInfo`, which is assigned from the `LedgerHandle#getLength()`. Therefore, the total size will become 0 or less before all ledgers are removed. ### Modifications - Update `dataLength` field in `setData` method. - Add a `testManagedLedgerTotalSize` test to `BrokerEntryMetadataE2ETest`. It produces 10 messages and trigger the rollover manually so that the first `LedgerInfo` of the managed ledger contains the correct total bytes. Then compare the `totalSize` field with it to verify this fix works. (cherry picked from commit 5dbb7d2)
…a is enabled (#12714) ### Motivation When the BrokerEntryMetadata is enabled, the total size in `ManagedLedgerImpl` is inaccurate. Because when the total size is updated in `OpAddEntry#safeRun`, the `dataLength` is the initial size of `data` when `OpAddEntry` is constructed, but `data` could be changed via `setData` method. The inaccurate total size could affect the retention size validation. Because in `ManagedLedgerImpl#internalTrimLedgers`, the total size reduces by the size of `LedgerInfo`, which is assigned from the `LedgerHandle#getLength()`. Therefore, the total size will become 0 or less before all ledgers are removed. ### Modifications - Update `dataLength` field in `setData` method. - Add a `testManagedLedgerTotalSize` test to `BrokerEntryMetadataE2ETest`. It produces 10 messages and trigger the rollover manually so that the first `LedgerInfo` of the managed ledger contains the correct total bytes. Then compare the `totalSize` field with it to verify this fix works. (cherry picked from commit 5dbb7d2)
…a is enabled (apache#12714) ### Motivation When the BrokerEntryMetadata is enabled, the total size in `ManagedLedgerImpl` is inaccurate. Because when the total size is updated in `OpAddEntry#safeRun`, the `dataLength` is the initial size of `data` when `OpAddEntry` is constructed, but `data` could be changed via `setData` method. The inaccurate total size could affect the retention size validation. Because in `ManagedLedgerImpl#internalTrimLedgers`, the total size reduces by the size of `LedgerInfo`, which is assigned from the `LedgerHandle#getLength()`. Therefore, the total size will become 0 or less before all ledgers are removed. ### Modifications - Update `dataLength` field in `setData` method. - Add a `testManagedLedgerTotalSize` test to `BrokerEntryMetadataE2ETest`. It produces 10 messages and trigger the rollover manually so that the first `LedgerInfo` of the managed ledger contains the correct total bytes. Then compare the `totalSize` field with it to verify this fix works.
…a is enabled (#12714) ### Motivation When the BrokerEntryMetadata is enabled, the total size in `ManagedLedgerImpl` is inaccurate. Because when the total size is updated in `OpAddEntry#safeRun`, the `dataLength` is the initial size of `data` when `OpAddEntry` is constructed, but `data` could be changed via `setData` method. The inaccurate total size could affect the retention size validation. Because in `ManagedLedgerImpl#internalTrimLedgers`, the total size reduces by the size of `LedgerInfo`, which is assigned from the `LedgerHandle#getLength()`. Therefore, the total size will become 0 or less before all ledgers are removed. ### Modifications - Update `dataLength` field in `setData` method. - Add a `testManagedLedgerTotalSize` test to `BrokerEntryMetadataE2ETest`. It produces 10 messages and trigger the rollover manually so that the first `LedgerInfo` of the managed ledger contains the correct total bytes. Then compare the `totalSize` field with it to verify this fix works. (cherry picked from commit 5dbb7d2)
Motivation
When the BrokerEntryMetadata is enabled, the total size in
ManagedLedgerImpl
is inaccurate. Because when the total size is updated inOpAddEntry#safeRun
, thedataLength
is the initial size ofdata
whenOpAddEntry
is constructed, butdata
could be changed viasetData
method.The inaccurate total size could affect the retention size validation. Because in
ManagedLedgerImpl#internalTrimLedgers
, the total size reduces by the size ofLedgerInfo
, which is assigned from theLedgerHandle#getLength()
. Therefore, the total size will become 0 or less before all ledgers are removed.Modifications
dataLength
field insetData
method.testManagedLedgerTotalSize
test toBrokerEntryMetadataE2ETest
. It produces 10 messages and trigger the rollover manually so that the firstLedgerInfo
of the managed ledger contains the correct total bytes. Then compare thetotalSize
field with it to verify this fix works.Verifying this change
This change added test
BrokerEntryMetadataE2ETest#testManagedLedgerTotalSize
.