[ROCKETMQ-45]Delete hanged consume queue files #39
Conversation
|
Hi, @lizhanhui , what's the appearance/effect of this BUG? Let's start from the next round. IMO, it's ok when ConsumeQueue-MappedFile hold failed, this file will be released by the last holder, while the empty mapped file will retain in MappedFileQueue, but the min consume queue offset will be corrected. It seems that the only problem is the hanged consume queue only can be deleted when restart the broker. |
| @@ -397,11 +397,21 @@ public int deleteExpiredFileByOffset(long offset, int unitSize) { | |||
| log.info("physic min offset " + offset + ", logics in current mappedFile max offset " | |||
| + maxOffsetInLogicQueue + ", delete it"); | |||
| } | |||
| } else if (!mappedFile.isAvailable()) { // Handle hanged file. | |||
There was a problem hiding this comment.
I think it's better to add RefCount to the if condition.
There was a problem hiding this comment.
Why? I do not see the rationale of awaiting RefCount to be 0 after commit log has been deleted after intervalForcibly period of time.
There was a problem hiding this comment.
When MappedFile has been shutdown, no more new holder appear, so the RefCount will to be 0 soon, so I think maybe it's more graceful :), but it's up to you~
There was a problem hiding this comment.
Another thing, can we just mark destroy=true in else if section and next if will destroy it ?
There was a problem hiding this comment.
Another thing, can we just mark destroy=true in else if section and next if will destroy it ?
Yep, I think we can do this.
Reported bug description is consume queue files cannot be automatically deleted and took up large portion of disk.
ConsumeQueue#correctMinOffset will not work as expected if consume queue file hanged. Double check the source code to figure out.
Not only this one. As correcting min offset is also affected, consumer client probably experiences long term of "commit log being deleted." before consuming progress catches up. There should be additional potential issues. |
|
I checked ConsumeQueue#correctMinOffset, and you are right this method won't work as expected in this situation. Please @vintagewang @vongosling help review this PR. |
|
@lizhanhui Also please add some unit tests -:).. |
2 similar comments
2 similar comments
|
Usually, we need 3 committers to review PR. We'd better not merge the PR if the opinion fails to unite |
Agree. But this is a minor change to fix obvious corner case mis-handling. Proposing review points are carefully considered and mostly accepted. Anyway, Let's wait for three review OK next time before merging. |
Consume queue file may hang and stop further deleting under concurrent scenario.
If the consume queue file is concurrently held by another thread, mappedFile.destroy(interval) will only set firstShutdownTimestamp to current timestamp and available false, then return false directly.
Check MappedFileQueue.java#deleteExpiredFileByOffset,
if (destroy && mappedFile.destroy(1000 * 60)) {files.add(mappedFile);deleteCount++;} else {break;}the current round of deletion will be stopped and current mapped file becomes handed.
Next time trying to delete mapped file from this mapped file queue, MappedFileQueue.java#deleteExpiredFileByOffset(offset, unitSize) line No.390
MappedFile mappedFile = (MappedFile) mfs[i];SelectMappedBufferResult result = mappedFile.selectMappedBuffer(this.mappedFileSize - unitSize);will always return null as the mapped file is no longer available and cannot hold. Current implementation will only log a warn and exit.