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

Fix multitag cardinality bug #842

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

yvanoers
Copy link
Collaborator

@yvanoers yvanoers commented Nov 2, 2020

Fixes #821.

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Good job here! thx @yvanoers, just one observation, I had a hard time reviewing because filterNodes function modifies execNodes inside, this is an unexpected change, I tend to prefer returning the modified structure for clarity, WDYT?

@yvanoers yvanoers force-pushed the fix-multitag-cardinality branch from 00280f1 to 608c02f Compare November 9, 2020 19:36
@yvanoers
Copy link
Collaborator Author

yvanoers commented Nov 9, 2020

I made the function return everything it touches. Also reduced the side effect of the tags changing and now returning new value.
I didn't want to create a new map to eliminate the execNodes side effect, because it would hit performance and mem use.

To really properly fix the lengthiness of these functions, I would create a separate struct with accompanying functions and put it all in a separate file. I didn't want to go that way, though, because it seems like an extensive solution to a relatively small problem.

Anyway, I pushed some changes, have a look and let me know your thoughts, @Victorcoder !

@yvanoers yvanoers requested a review from vcastellm November 9, 2020 19:42
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

This looks much more intuitive, thanks!

@vcastellm vcastellm merged commit cbc6c49 into distribworks:master Nov 9, 2020
@yvanoers yvanoers deleted the fix-multitag-cardinality branch November 9, 2020 20:29
@vcastellm
Copy link
Member

Hi @yvanoers I'm receiving reports of this change affecting the evenly balancing of executions in nodes
image

I did not have verified it myself, but I'm letting you know before looking at it, because if you see a quick fix.

@yvanoers
Copy link
Collaborator Author

@Victorcoder Interesting, I'll have a look tonight.
Am I correct in assuming that the graph represents the number of jobs executed on 3 different servers (a different color for each server)?

@vcastellm
Copy link
Member

@yvanoers yes, correct, it changed colors because of node restart, switching leader, jobs are mostly executed on the leader

@yvanoers
Copy link
Collaborator Author

@Victorcoder I found the problem, I will create a pull request shortly.

@yvanoers yvanoers mentioned this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent 'no target nodes found to run job'
2 participants