Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

cluster: drop node updates that are old or about thisNode #948

Merged
merged 4 commits into from
Oct 29, 2018
Merged

Conversation

woodsaj
Copy link
Member

@woodsaj woodsaj commented Jun 22, 2018

fixes #947

Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

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

so this still allows a node to receive gossiped information about itself. is there ever a scenario where this makes sense? seems clearer if we would just discard all data about itself. then we know for sure we can't have the reported bug.

@@ -348,6 +354,7 @@ func (c *MemberlistManager) SetState(state NodeState) {
node := c.members[c.nodeName]
node.State = state
node.Updated = time.Now()
node.StateChange = time.Now()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add comments to the HTTPNode definition about what Updated and StateChange represent / how they are used? it's not clear how they differ

Copy link
Member Author

Choose a reason for hiding this comment

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

really? one is set when the httpNode is updated one is set when the httpNodes's state changes

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, really.
It's more about why things are implemented this way. i.e. the overarching design of the code.
there is a lot of information in your head that is not there for others reading your code.

for example:

  • why is StateChange never read from in the code, is it only for inspection by admin?
  • why does changing the State and StateChange fields not trigger a gossip broadcast whereas changing Update would? (error clause in MemberlistManager.NotifyUpdate)
  • who is responsible for updating the fields: the local nodes emitting their state, or the peers receiving it? (seems to be both?)
  • does "update" include an update to the state field? seems to be yes except in the above mentioned exception case?

documentation helps in answering these sort of questions.

BTW should SingleNodeManager.SetState not update StateChange ?

@Dieterbe
Copy link
Contributor

@woodsaj

so this still allows a node to receive gossiped information about itself. is there ever a scenario where this makes sense? seems clearer if we would just discard all data about itself. then we know for sure we can't have the reported bug.

@Dieterbe
Copy link
Contributor

Dieterbe commented Sep 7, 2018

awoods [11:28 AM]
I can't think of any reason why we would want gossiped messages to alter the state of thisNode

@Dieterbe
Copy link
Contributor

i'll rebase this and get this ready again.

@Dieterbe
Copy link
Contributor

Dieterbe commented Oct 29, 2018

@woodsaj how does this look

note that we should still clarify #948 (comment)

@woodsaj
Copy link
Member Author

woodsaj commented Oct 29, 2018

@Dieterbe LGTM

@Dieterbe Dieterbe merged commit d792236 into master Oct 29, 2018
@Dieterbe Dieterbe changed the title drop node updates that are older then info we already have cluster: drop node updates that are old or about thisNode Oct 29, 2018
@Dieterbe Dieterbe added this to the 0.11.0 milestone Dec 12, 2018
@Dieterbe Dieterbe deleted the issue947 branch March 27, 2019 21:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sometimes pod doesn't become ready. I think because of problem with .State attribute
2 participants