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

Merge fix for node stall ready search #38

Merged
merged 5 commits into from
Dec 5, 2024

Conversation

BLuedtke
Copy link
Collaborator

@BLuedtke BLuedtke commented May 6, 2024

Closes #28

Draft because tests still have to be run on how this affects overall operation. More info available in issue #28

@BLuedtke BLuedtke self-assigned this May 6, 2024
@BLuedtke BLuedtke changed the title 28 node stall ready check fix Merge fix for node stall ready search May 6, 2024
@BLuedtke
Copy link
Collaborator Author

BLuedtke commented Dec 4, 2024

Looking at it again, it is now clear to me why this worked before the fix.

The point of searching the stall_affected_nodes_queue was to see if a node was already in there - if not, add it. Later, in bidib_node_update_stall, when the node is not stalled anymore, all entries in the stall_affected_nodes_queue are popped and for each entry, we try to send any queued messages.

With the old code where the search function for the stall_affected_nodes_queue did not work, it would always return false -> so the node might be added to the stall affected nodes queue multiple times. But that just means that when the node is not stalled anymore, and it has multiple identical entries in the stall_affected_nodes_queue, it just tries to send queued messages multiple times, which is no problem.

So the fix only prevents the same node from being added to the stall affected nodes queue multiple times, nothing else.

@BLuedtke BLuedtke marked this pull request as ready for review December 4, 2024 13:38
@BLuedtke BLuedtke requested a review from eyip002 December 4, 2024 13:39
eyip002
eyip002 previously approved these changes Dec 4, 2024
Copy link
Member

@eyip002 eyip002 left a comment

Choose a reason for hiding this comment

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

Changes look good. Minor suggestion for a comment.

src/transmission/bidib_transmission_node_states.c Outdated Show resolved Hide resolved
@BLuedtke
Copy link
Collaborator Author

BLuedtke commented Dec 5, 2024

Huh? Not sure why it says I dismissed the review. Let me see if I can revert that.

Edit: Can't see where I can revert it. Makes no sense, not sure how this happened. GitHub is acting weird wrt. merge requests recently, sometimes it links completely unrelated issues automatically.

@BLuedtke BLuedtke merged commit 831ba48 into master Dec 5, 2024
1 check passed
@BLuedtke BLuedtke deleted the 28-node-stall-ready-check-fix branch December 5, 2024 08:42
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.

Node Stall Ready check does not perform search in stall queue as intended
2 participants