New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

msg/async: avoid lossy connection sending ack message #13700

Merged
merged 1 commit into from Mar 14, 2017

Conversation

Projects
None yet
4 participants
@yuyuyu101
Member

yuyuyu101 commented Feb 28, 2017

Signed-off-by: Haomai Wang haomai@xsky.com

@yuyuyu101 yuyuyu101 requested a review from liewegas Mar 1, 2017

@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Mar 3, 2017

@liewegas sorry, we must rebuild testing branch since previous codes:
if (policy.lossy) {
ack_left.inc();
need_dispatch_writer = true;
}

It's totally wrong. I updated this commit!

@yuriw

This comment has been minimized.

Contributor

yuriw commented Mar 5, 2017

test this pllese

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 6, 2017

@yuyuyu101 could you rebase against the latest master and try again, the failure was fixed in fec59e8

msg/async: avoid lossy connection sending ack message
Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Mar 6, 2017

hmm, from the log I don't think it's related to this pr... maybe I need to figure out when this test failed

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 6, 2017

after reverting this change, the test passes. maybe this change reveals another issue?

@yuriw yuriw removed the wip-yuri-testing label Mar 6, 2017

@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Mar 7, 2017

I can't reproduce this error locally. Could you run it again ?

@yuriw

This comment has been minimized.

Contributor

yuriw commented Mar 7, 2017

Trying to rebuild and test again

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 13, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 14, 2017

ahh, we were testing the buggy commit

commit 854c3713651a036342865a4f0c87e88f5cb75d05
Author: Haomai Wang <haomai@xsky.com>
Date:   Wed Mar 1 00:16:55 2017 +0800

    msg/async: avoid lossy connection sending ack message

    Signed-off-by: Haomai Wang <haomai@xsky.com>

diff --git a/src/msg/async/AsyncConnection.cc b/src/msg/async/AsyncConnection.cc
index 0dc74d3..c8db137 100644
--- a/src/msg/async/AsyncConnection.cc
+++ b/src/msg/async/AsyncConnection.cc
@@ -768,8 +768,10 @@ void AsyncConnection::process()
                                     << message->get_seq() << " " << message
                                    << " " << *message << dendl;

-          ack_left.inc();
-          need_dispatch_writer = true;
+          if (policy.lossy) {
+            ack_left.inc();
+            need_dispatch_writer = true;
+          }
           state = STATE_OPEN;

           logger->inc(l_msgr_recv_messages);

with the updated change, the test passes.

@tchaikov tchaikov merged commit 16b03c4 into ceph:master Mar 14, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@yuyuyu101 yuyuyu101 deleted the yuyuyu101:wip-msgr-lossy branch Mar 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment