dd-xiaozhi opened a new pull request, #11497:
URL: https://github.com/apache/rocketmq/pull/11497
### Which Issue(s) This PR Fixes
- Fixes #9854
### Brief Description
`CleanCommitLogService#deleteExpiredFiles` calls the
`CommitLog#deleteExpiredFile` overload that takes `deleteFileBatchMax`:
```java
deleteCount =
DefaultMessageStore.this.commitLog.deleteExpiredFile(fileReservedTime,
deletePhysicFilesInterval,
destroyMappedFileIntervalForcibly, cleanAtOnce, deleteFileBatchMax);
```
But `DLedgerCommitLog` only overrides the 4-argument overload. As a result,
the 5-argument call goes straight to `CommitLog` →
`mappedFileQueue.deleteExpiredFileByTime(...)`, and the DLedger-specific logic
in the override never runs in a broker:
1. In a pure DLedger deployment the old `mappedFileQueue` is empty, so the
call returns `0` instead of the intended `Integer.MAX_VALUE`. Whenever disk
usage exceeds the threshold, the broker keeps logging `disk space will be full
soon, but delete file failed.`, which is the warning shown in #9854.
2. `refreshConfig()` is never invoked, so `fileReservedTime` / `deleteWhen`
/ `cleanFileForciblyEnable` changed at runtime (e.g. via `updateBrokerConfig`)
are not propagated to the DLedger store.
3. In a mixed deployment (upgraded from a normal commitlog),
`disableDeleteDledger()` is not applied and the last old commitlog file is
never removed.
This PR changes the override to the 5-argument overload. The 4-argument
overload in `CommitLog` already delegates to the 5-argument one, so both entry
points now go through the DLedger override.
Note: the main symptom of #9854 on JDK 9+ (expired DLedger files not being
deleted) was addressed by #9851. This PR fixes the remaining part: the bypassed
override behind the misleading warning and the missing config refresh.
### How Did You Test This Change?
- Added `MixCommitlogTest#testDeleteExpiredFilesWithBatchMax`, which calls
the same overload as `CleanCommitLogService`. Without the fix it fails with
`expected:<2147483647> but was:<0>`; with the fix it passes. It also verifies
that a runtime change of `cleanFileForciblyEnable` is refreshed into the
DLedger config.
- The existing `MixCommitlogTest#testDeleteExpiredFiles` (mixed deployment,
4-argument entry) still passes, now going through the override via delegation.
- `mvn -pl store test
-Dtest='MixCommitlogTest,DLedgerDeprecationTest,DLedgerMultiPathTest,DLedgerCommitlogTest'`
passes, and checkstyle reports no violations.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
