Skip to content

[ISSUE #1904] Print log when flush timeout#1903

Merged
RongtongJin merged 3 commits intoapache:developfrom
rushsky518:timeout_msg
Dec 4, 2020
Merged

[ISSUE #1904] Print log when flush timeout#1903
RongtongJin merged 3 commits intoapache:developfrom
rushsky518:timeout_msg

Conversation

@rushsky518
Copy link
Copy Markdown
Contributor

What is the purpose of the change

Print log when flush timeout

@rushsky518
Copy link
Copy Markdown
Contributor Author

#1904

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 1, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@c5e5f8d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1903   +/-   ##
==========================================
  Coverage           ?   45.10%           
  Complexity         ?     4198           
==========================================
  Files              ?      544           
  Lines              ?    35732           
  Branches           ?     4746           
==========================================
  Hits               ?    16116           
  Misses             ?    17566           
  Partials           ?     2050           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5e5f8d...80ebef3. Read the comment docs.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 1, 2020

Coverage Status

Coverage decreased (-0.009%) to 50.905% when pulling 80ebef3 on rushsky518:timeout_msg into 0a9822e on apache:develop.

@ShannonDing ShannonDing changed the title Print log when flush timeout [ISSUE #1904] Print log when flush timeout Apr 1, 2020
@ShannonDing
Copy link
Copy Markdown
Member

It seems to conflict with #1894, could you please check again?

if (flushStatus != PutMessageStatus.PUT_OK) {
putMessageResult.setPutMessageStatus(PutMessageStatus.FLUSH_DISK_TIMEOUT);
putMessageResult.setPutMessageStatus(flushStatus);
log.error("do groupcommit, wait for flush failed, topic: {} tags: {} client address: {}",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the disk flashing has failed, and continue to log here, it will increase the system load and increase disk IO.
IMO, as the status will be sent to the client, it is better to remove the error log here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed so.
I still have a question about [#1894 pr]

req.wakeupCustomer(flushOK ? PutMessageStatus.PUT_OK : PutMessageStatus.FLUSH_DISK_TIMEOUT);

flushOK is false means FLUSH_DISK_TIMEOUT?

@RongtongJin RongtongJin merged commit bb9f106 into apache:develop Dec 4, 2020
@vongosling
Copy link
Copy Markdown
Member

Need to associate with milestone :-) @RongtongJin

@RongtongJin RongtongJin added this to the 4.8.0 milestone Dec 5, 2020
GenerousMan pushed a commit to GenerousMan/rocketmq that referenced this pull request Aug 12, 2022
* rollback my code

* avoid log when disk flush

* when msg is in next file, flushOK value may be wrong
pulllock pushed a commit to pulllock/rocketmq that referenced this pull request Oct 19, 2023
* rollback my code

* avoid log when disk flush

* when msg is in next file, flushOK value may be wrong
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get flushStatus from CompletableFuture, error log is missed when flush timeout

7 participants