Skip to content
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

[DO NOT REVIEW] #4990

Closed
wants to merge 12 commits into from
Closed

[DO NOT REVIEW] #4990

wants to merge 12 commits into from

Conversation

jujoramos
Copy link
Contributor

No description provided.

Comment on lines +165 to +166
Long firstHeartbeatServer1 = server1.invoke(this::getSingleHeartBeat);
Long firstHeartbeatServer2 = server2.invoke(this::getSingleHeartBeat);
Copy link
Contributor

Choose a reason for hiding this comment

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

@jujoramos There is something I dont get here: if there is a ClientHealthMonitor per server, how it is possible that a ping send for distributedMember2 creates an entry in the ClientHealthMonitor of server1? I have been debugging this and I only see one call in ClientHealthMonitor::receivedPing in server 2 (as expected) after executing the ping (line 164). Could it be a problem of the testing framework? Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @alb3rtobr
Sorry for the confusion here, the test memberShouldCorrectlyRedirectPingMessage is not finished at all, I was just using it to debug the issue.

Copy link
Contributor

@alb3rtobr alb3rtobr Apr 28, 2020

Choose a reason for hiding this comment

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

Ok! Anyway I found a possible cause for the unexpected heartbeat. I think the assert in line 160 is not correct. Sometimes the test fails there, although no ping has been sent yet. The ServerConnection::processHandshake method calls ClientHealthMonitor::registerClient, which will cause that ClientHealthMonitor.getClientHeartbeats() will not be empty. I think sometimes the test is fast enough to pass the assert, and the heartbeat got in line 165 is the one created by ServerConnection::processHandshake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, do not worry about this particular test method, the one showing the issue is memberShouldNotRedirectPingMessageWhenClientCachedViewIdIsWrong. You can use the other two as starting points to develop your own tests for the GEODE-7565.

@jujoramos jujoramos closed this May 5, 2020
@jujoramos jujoramos deleted the feature/GEM-2885 branch May 5, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants