Skip to content

DISPATCH-1096 - priority sheaf cleanup bug#384

Closed
mgoulish wants to merge 2 commits intoapache:masterfrom
mgoulish:priority_cleanup_bug
Closed

DISPATCH-1096 - priority sheaf cleanup bug#384
mgoulish wants to merge 2 commits intoapache:masterfrom
mgoulish:priority_cleanup_bug

Conversation

@mgoulish
Copy link

This is for the bug that Ganesh and Chuck saw while killing and restarting routers.
The test succeeded 10 times each (with 0 false results) in these 4 cases:
{ Debug, RelWithDebInfo } x { Should_Fail, Should_Succeed }

@codecov-io
Copy link

codecov-io commented Sep 26, 2018

Codecov Report

Merging #384 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #384      +/-   ##
=========================================
+ Coverage    84.8%   84.8%   +<.01%     
=========================================
  Files          73      73              
  Lines       16462   16466       +4     
=========================================
+ Hits        13960   13964       +4     
  Misses       2502    2502
Impacted Files Coverage Δ
src/router_core/router_core.c 92.18% <100%> (-0.23%) ⬇️
src/router_core/connections.c 93.17% <100%> (-0.11%) ⬇️
src/container.c 76.55% <0%> (-0.2%) ⬇️
src/router_core/core_test_hooks.c 88.06% <0%> (+1.7%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa03ea4...28f08a2. Read the comment docs.

@ChugR
Copy link
Contributor

ChugR commented Sep 26, 2018

This patch clears up the too-many-links for sheaf error and improves my router network stability. FOr this I approve the patch.

But in tests before and after this patch my throughput went from 2600 msg/S to 200-300 msg/S. The performance drop needs to be addressed as a separate issue.

Copy link
Member

Choose a reason for hiding this comment

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

Exiting the process here is a bit heavy handed. I think it's sufficient to log the critical error.

@ted-ross
Copy link
Member

This patch clears up the too-many-links for sheaf error and improves my router network stability. FOr this I approve the patch.

But in tests before and after this patch my throughput went from 2600 msg/S to 200-300 msg/S. The performance drop needs to be addressed as a separate issue.

I don't see any changes in this PR that should affect throughput performance.

@ted-ross
Copy link
Member

This patch clears up the too-many-links for sheaf error and improves my router network stability. FOr this I approve the patch.
But in tests before and after this patch my throughput went from 2600 msg/S to 200-300 msg/S. The performance drop needs to be addressed as a separate issue.

I don't see any changes in this PR that should affect throughput performance.

I withdraw the above statement. There appears to be a memory leak that is causing performance (and other) problems.

@mgoulish
Copy link
Author

I yanked that exit(1).
I thought I needed that to detect failure in my python test, but that is not true.
Tests all still did the right thing, in 10 trials each (40 total) in the set: {should_fail, should_succeed} * { DEBUG, RelWithDebInfo}

@asfgit asfgit closed this in e813f08 Oct 11, 2018
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.

4 participants

Comments