fix retention size policy delete too much ledgers #11242
Conversation
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| if (hasLedgerRetentionExpired(ls.getTimestamp())) { |
There was a problem hiding this comment.
I'm not sure why the PR can fix the problem. Looks the problem is the existing logic is if the ledger over retention quota or expired, the ledger will be added into the ledgersToDelete, the correct logic is if the ledger over retention quota and expired, we should add it into the ledgersToDelete right?
But looks like the new change still adds the ledger to the ledgersToDelete after over retention quota without checking the expired.
There was a problem hiding this comment.
the correct logic is if the ledger over retention quota and expired, we should add it into the ledgersToDelete right?
I think the logic of retention policy means either over retention quota or expired, the ledger should be delete.
There was a problem hiding this comment.
Oh. sorry, My mistake.
| ManagedLedgerFactory factory = new ManagedLedgerFactoryImpl(metadataStore, bkc); | ||
| ManagedLedgerConfig config = new ManagedLedgerConfig(); | ||
| config.setRetentionSizeInMB(retentionSizeInMB); | ||
| config.setMaxEntriesPerLedger(1); |
There was a problem hiding this comment.
We have a minRolloverTime limitation by default 10min, Set max 1 entry per ledger will not trigger the ledger rollover.
There was a problem hiding this comment.
The default minimumRolloverTimeMs is 0
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerTest.java
Show resolved
Hide resolved
|
/pulsarbot run-failure-checks |
| } | ||
| } | ||
|
|
||
| if (hasLedgerRetentionExpired(ls.getTimestamp())) { |
There was a problem hiding this comment.
Oh. sorry, My mistake.
|
Nice catch @aloyszhang |
Fixes apache#11241 This pull request fix the error that retention size policy delete too much ledgers when all cursors has no backlog 1. generate `ledgersToDelete` by checking ledger size one by one instead of `TOTAL_SIZE_UPDATER` 2. refactor the logic of generate `ledgersToDelete` 3. add test code (cherry picked from commit 8ca6f19)
Fixes #11241 ### Motivation This pull request fix the error that retention size policy delete too much ledgers when all cursors has no backlog ### Modifications 1. generate `ledgersToDelete` by checking ledger size one by one instead of `TOTAL_SIZE_UPDATER` 2. refactor the logic of generate `ledgersToDelete` 3. add test code (cherry picked from commit 8ca6f19)
Fixes apache#11241 ### Motivation This pull request fix the error that retention size policy delete too much ledgers when all cursors has no backlog ### Modifications 1. generate `ledgersToDelete` by checking ledger size one by one instead of `TOTAL_SIZE_UPDATER` 2. refactor the logic of generate `ledgersToDelete` 3. add test code (cherry picked from commit 8ca6f19) (cherry picked from commit 3ad707b)
Fixes apache#11241 ### Motivation This pull request fix the error that retention size policy delete too much ledgers when all cursors has no backlog ### Modifications 1. generate `ledgersToDelete` by checking ledger size one by one instead of `TOTAL_SIZE_UPDATER` 2. refactor the logic of generate `ledgersToDelete` 3. add test code
Fixes #11241
Motivation
This pull request fix the error that retention size policy delete too much ledgers when all cursors has no backlog
Modifications
ledgersToDeleteby checking ledger size one by one instead ofTOTAL_SIZE_UPDATERledgersToDeleteVerifying this change
This change added tests and can be verified as follows:
ManagedLedgerTest.testRetentionSize()Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation