-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[ISSUE #4925] correct the member's state when the cluster information… #4948
Conversation
…rmation changed
member.setState(NodeState.DOWN); | ||
} else { | ||
//fix issue # 4925 | ||
member.setState(serverList.get(address).getState()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a right fix.
this method is update members, so we should use input member state I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we could use input member state in this method
synchronized boolean memberChange(Collection<Member> members) {}
This method is only called in three places:
1.
In this case, because the member state is set to UP by default, whether it's from cluster.conf or address-server.
If a member has been judged as down, it will be reset to UP after this method. So this issue 4925 occured.
In this case, a joined member is not in the serverList. We are not sure if the joined member's process has started, so it should be set to DOWN at first, and set to UP after MemberInfoReportTask completed.
This has no effect on the method memberLeave()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether it will affect other report or update states workflow. I think we need do some discussion.
@chuntaojun @shiyiyue1102 How do you think about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only the state value of the member, I think all the properties of the member should be based on the value in the serverlist.
Otherwise, the data returned by the interface /nacos/v1/ns/operator/servers is always changing (be reset),for the same reason as issue 4925.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But some of other Member attributes should modified by other members‘ report.
So I think the final value should be updated from the inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is temporary plan, finally we should figure out the remote data update and local data update to solve this problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But some of other Member attributes should modified by other members‘ report.
So I think the final value should be updated from the inputs.
There is no problem that final value should be updated from the inputs, because this method
memberChange(Collection<Member> members)
is not called when nacos-server received other members' report.
It seems that this method is only called when the cluster information is updated,I think the attribute value of member should be based on the value in nacos-server memory.
KomachiSion 的担心是对的,我们就碰上#4948修订导致的问题: |
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
try to fix issue #4925
Brief changelog
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.