You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by lizhanhui <gi...@git.apache.org> on 2017/01/12 07:07:49 UTC
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
GitHub user lizhanhui opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/39
[ROCKETMQ-45]Delete hanged consume queue files
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lizhanhui/incubator-rocketmq ROCKETMQ-45
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/39.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #39
----
commit 4bb113ce87c6ff52fc9af7ddf2a389e2a45853b6
Author: Zhanhui Li <li...@apache.org>
Date: 2017-01-12T07:02:26Z
Delete hanged consume queue files
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/39#discussion_r95979009
--- Diff: store/src/main/java/org/apache/rocketmq/store/MappedFileQueue.java ---
@@ -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.
--- End diff --
Another thing, can we just mark `destroy=true` in `else if` section and next `if` will destroy it ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
[](https://coveralls.io/builds/9784804)
Coverage increased (+0.1%) to 25.854% when pulling **b8e53c990e0b467c2a2b766137f7b32f3b5b6f04 on lizhanhui:ROCKETMQ-45** into **b29c318cdde225ef3a33a73e939e49e087766a28 on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
Usually, we need 3 committers to review PR. We'd better not merge the PR if the opinion fails to unite
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
> what's the appearance/effect of this BUG?
Reported bug description is consume queue files cannot be automatically deleted and took up large portion of disk.
> 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.
ConsumeQueue#correctMinOffset will not work as expected if consume queue file hanged. Double check the source code to figure out.
> It seems that the only problem is the hanged consume queue only can be deleted when restart the broker.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/39#discussion_r95964644
--- Diff: store/src/main/java/org/apache/rocketmq/store/MappedFileQueue.java ---
@@ -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.
--- End diff --
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 :)..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
[](https://coveralls.io/builds/9784819)
Coverage increased (+0.1%) to 25.814% when pulling **57111a02721ac8d70de3bb6210db4cf13f4714a9 on lizhanhui:ROCKETMQ-45** into **b29c318cdde225ef3a33a73e939e49e087766a28 on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/39#discussion_r97212223
--- Diff: store/src/main/java/org/apache/rocketmq/store/MappedFileQueue.java ---
@@ -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.
--- End diff --
> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/39#discussion_r95955059
--- Diff: store/src/main/java/org/apache/rocketmq/store/MappedFileQueue.java ---
@@ -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.
--- End diff --
I think it's better to add `RefCount` to the if condition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/39
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #39: [ROCKETMQ-45]Delete hanged consume queu...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/39#discussion_r95959190
--- Diff: store/src/main/java/org/apache/rocketmq/store/MappedFileQueue.java ---
@@ -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.
--- End diff --
Why? I do not see the rationale of awaiting `RefCount` to be 0 after commit log has been deleted after `intervalForcibly` period of time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #39: [ROCKETMQ-45]Delete hanged consume queue files
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/39
@lizhanhui Also please add some unit tests -:)..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---