Refactor processFilteredNodes for testing #968
Conversation
I was digging into the intermittent failures of Test_processFilteredNodes and had trouble really understanding what the function, and the complimenting filterNodes were doing so I broke some of the logic out into smaller functions to encapsulate the logic and make testing of each portion easier. I added unit tests for these smaller functions to help catch potential issues with each. I left Test_processFilteredNodes as it serves well as a higher level test of all the other funtions. This adjusts the thresholds for the even distribution of picking nodes when the cardinality is lower than the possible nodes. Due to the randomness, it is quite hard to test how evenly distributed the values are within the original thresholds. This would cause many intermittent test failures when the values were just over or under the thresholds. This also moves the seed of math.Rand into the agent startup. Reseeding the randomness every function call seems like extra overhead for no value. I do not believe that constantly reseeding the randomness increases randomness in anyway.
There was a problem hiding this comment.
Nice work, certainly more readable.
Though I do suspect this is not as fast, but it would have to be measured to be sure and assess whether or not difference is significant, especially for larger clusters. This concern is the reason why I didn't pull it apart when last I touched this.
|
I didn't realize the intermittent failures of the filterNodes test were frequent enough to make someone investigate it. I will intensify my efforts to fix the failures. |
|
Thanks for the review @yvanoers. I incorporated your feedback in another commit. I'm not sure how many people are regularly running the tests to catch the intermittent failures, but I have been running them a ton recently because of my work on race conditions in tests. I noticed in another one of your PRs that you closed and reopened it after the same failing test that I had in before. That leads me to believe that there are still some flaky tests beyond what this PR intends to solve. |
Oh yes, there definitely is. I know exactly where the problem lies and I have a solution waiting in a branch (as sort-of promised a couple days ago). But it would conflict with this PR. If this PR gets merged I can go ahead and finish my changes. Or if it gets rejected, there are a couple of changes in this PR I'd like to incorporate. Edit: Oh wait, there are other tests than the one I'm targetting that are failing too - I didn't realize until now. |
dkron/agent.go
Outdated
| // * the (modified) map of execNodes | ||
| // * a map of tag values without cardinality | ||
| // * the map of execNodes that match the provided tags | ||
| // * a clean map of tag values without cardinality | ||
| // * cardinality, i.e. the max number of nodes that should be targeted, regardless of the | ||
| // number of nodes in the resulting map. | ||
| // * an error if a cardinality was malformed | ||
| func filterNodes(execNodes map[string]serf.Member, tags map[string]string) (map[string]serf.Member, map[string]string, int, error) { |
There was a problem hiding this comment.
Maybe execNodes -> allNodes? but I'm nitpicking
|
I think that's a more readable name @Victorcoder. I just pushed the update. Thanks! |
I was digging into the intermittent failures of Test_processFilteredNodes and had trouble really understanding what the function, and the complimenting filterNodes were doing so I broke some of the logic out into smaller functions to encapsulate the logic and make testing of each portion easier. I added unit tests for these smaller functions to help catch potential issues with each. I left Test_processFilteredNodes as it serves well as a higher level test of all the other funtions.
This adjusts the thresholds for the even distribution of picking nodes when the cardinality is lower than the possible nodes. Due to the randomness, it is quite hard to test how evenly distributed the values are within the original thresholds. This would cause many intermittent test failures when the values were just over or under the thresholds.
This also moves the seed of math.Rand into the agent startup. Reseeding the randomness every function call seems like extra overhead for no value. I do not believe that constantly reseeding the randomness increases randomness in anyway.
This may help the tests for #967