#2033: lb: rewrite and fix bugs in OfflineLB/LBDataRestartReader #2034
Conversation
|
Tests are coming. |
lifflander
force-pushed
the
2033-simplify-and-fix-offlinelb
branch
from
7042e87 to
301e21d
Compare
lifflander
force-pushed
the
2033-simplify-and-fix-offlinelb
branch
from
301e21d to
a82b78c
Compare
Codecov Report
@@ Coverage Diff @@
## develop #2034 +/- ##
===========================================
+ Coverage 84.75% 84.86% +0.11%
===========================================
Files 722 723 +1
Lines 25600 25549 -51
===========================================
- Hits 21696 21683 -13
+ Misses 3904 3866 -38
|
lifflander
force-pushed
the
2033-simplify-and-fix-offlinelb
branch
from
9a187c0 to
9bace2f
Compare
| for (PhaseType phase = 0; phase < num_phases_; phase++) { | ||
| auto iter = lbdh.node_data_.find(phase); | ||
| if (iter == lbdh.node_data_.end()) { | ||
| history_[phase] = history_[prev_known_phase]; |
There was a problem hiding this comment.
This is an interesting solution to the problem. Could we end up using too much memory this way? My original thought was that the LB interval would likely not be 1, so only a subset of the phases would be needed anyway. I think it's also possible to mix OfflineLB with another load balancer (on different phases) by using an LB config file.
There was a problem hiding this comment.
Perhaps we should only store the phases for which the LB configuration would use OfflineLB?
There was a problem hiding this comment.
I agree that it uses more memory than necessary. It was just the easiest way to write the code. I can do a search each time instead that will find the last previous known phase.
There was a problem hiding this comment.
Ok based on our conversation, for the sake of this PR, we are going to assume that the phase specification is dense is fully specified for every phase up to num_phases.
lifflander
force-pushed
the
2033-simplify-and-fix-offlinelb
branch
from
9bace2f to
4905eda
Compare
Fixes #2033