[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16176648#comment-16176648
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/20
@shroman It seems that last cleaning, for too long-time no-resolved prs,
included this one.
If you feel it is necessary to reopen this PR, please feel free to do it.
you may also could create a new PR or launch new discussion in the mailing
list.
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Improvement
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
> Fix For: 4.2.0-incubating
>
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16172736#comment-16172736
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/20
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Improvement
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
> Fix For: 4.2.0-incubating
>
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16171037#comment-16171037
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/20
@shroman could you please resolve the conflicts?
IMO, the return value of flush() and commit(), means whether there is data
left to be flushed or committed, that is OK.
if you'd like to polish the semantic of the returned value, may you do the
same polishing for commit() too, just to keep the same semantic for similar
methods.
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Improvement
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
> Fix For: 4.2.0-incubating
>
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784792#comment-15784792
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/20
@shroman
what are your recommendations on how to treat the storage module? Should I
create a JIRA issue if I find a problem, but not work on it?
yeah, if you find some bugs or improvement, you can raise it in JIRA, just
later work on it :-) we could start a thread in the email list, to discuss how
to optimize here in the latter version.
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Bug
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
> Fix For: 4.1.0-incubating
>
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784776#comment-15784776
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/20
Ok, guys, I set the fix version of the JIRA issue to 4.1.0 then
@vongosling what are your recommendations on how to treat the storage
module? Should I create a JIRA issue if I find a problem, but not work on it?
Or should I create a PR, but it would be merged only in 4.1.0? Just curious,
why merging the fixes to it is better for later versions?
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Bug
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
> Fix For: 4.1.0-incubating
>
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15783125#comment-15783125
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/20#discussion_r94045317
--- Diff: store/src/main/java/org/apache/rocketmq/store/CommitLog.java ---
@@ -964,9 +964,11 @@ public void run() {
boolean result = false;
for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
--- End diff --
Consider `result = flush(0)`, the false result means there may be more data
need to flush, so retry. Otherwise, the true result means all data are flushed,
the broker can exit safely.
So if you change `result`, also please change this `for` loop.
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Bug
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15782769#comment-15782769
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/20
Hi, the github says "This branch has no conflicts with the base branch"
I think conflicts are resolved.
Reviewing it by several committers is already being cautious, isn't it? ;)
This issue looks much like a pretty nasty bug.
Anyway, looking forward to your reviews ;)
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Bug
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (ROCKETMQ-23) MappedFileQueue#flush should return true when flushing is successful
[
https://issues.apache.org/jira/browse/ROCKETMQ-23?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15782746#comment-15782746
]
ASF GitHub Bot commented on ROCKETMQ-23:
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/20
Hi, shroman
Could you please resolve conflicts in this pr?
And we will remain cautious about the changes of rocketmq-store module, so
this pr may be won't merge quickly.
Thanks.
> MappedFileQueue#flush should return true when flushing is successful
>
>
> Key: ROCKETMQ-23
> URL: https://issues.apache.org/jira/browse/ROCKETMQ-23
> Project: Apache RocketMQ
> Issue Type: Bug
> Components: rocketmq-store
>Affects Versions: 4.0.0-incubating
>Reporter: Roman Shtykh
>Assignee: Roman Shtykh
>
> In the current implementation, MappedFileQueue#flush returns {{false}} when
> flushing is successful.
> This is not intuitive and error prone.
> For instance, in {{CommitLog#run line:915-918}}
> {code}
> for (int i = 0; i < RETRY_TIMES_OVER && !result; i++) {
>result = CommitLog.this.mappedFileQueue.flush(0);
>// ...
> }
> {code}
> I believe retries has to be done when flushing is not successful. But with
> the code above, it can try to flush only once on CommitLog termination and
> not continue retrying.
> The same is for {{DefaultMessageStore#doFlush line:1551}}
> Or is this not retry on failure, but the number of times flushing has to be
> done? Then, {{RETRY_TIMES_OVER}} should be renamed to something like
> {{FLUSH_NUM}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
