1469 Output LB statistics as JSON #1475
Conversation
lifflander
requested review from
cz4rs,
nmm0,
jstrzebonski,
JacobDomagala,
nlslatt,
PhilMiller,
ppebay and
marcinwrobel1986
|
@PhilMiller @JacobDomagala @jstrzebonski @ppebay @nlslatt I would like feedback on this PR! |
| std::unique_ptr<StatsData> sd | ||
| ) { | ||
| std::deque<std::set<ElementIDType>> element_history; | ||
| for (PhaseType phase = 0; phase < sd->node_data_.size(); phase++) { |
There was a problem hiding this comment.
Would you be able to convert this loop into for-each? Maybe something like this:
for (auto const& nd : sd->node_data_) {
There was a problem hiding this comment.
The problem is that sd->node_data_ is an unordered_map but these need to be traversed in the correct order for element_history to be correct. So I can't use an iterator.
|
|
||
| template <typename StreamLike> | ||
| bool Compressor::writeImpl( | ||
| StreamLike& s, uint8_t const* buffer, std::size_t const size, bool finish_ |
There was a problem hiding this comment.
I see the point in the usage of finish_, but to be honest, I'm not a fan of changing the behaviour of the function with a boolean variable. But from the top of my head, I don't have any clever idea of refactoring this function. So I'm fine with leaving it be.
There was a problem hiding this comment.
I initially had two different functions (one for finishing and one for incremental writes). The problem is that I have to duplicate basically this whole function so I combined them.
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
this makes me think that extracting some helpers could help readability
lifflander
force-pushed
the
1469-lb-stats-json
branch
from
0e952a9 to
1852358
Compare
Fixes #1469
Should we completely remove the old stat output or leave it as an option that the user can select until we decide to remove it?