Fix a bunch of bugs plus an improvement to log_parser that I used to debug this.#302
Open
kumpera wants to merge 6 commits into
Open
Fix a bunch of bugs plus an improvement to log_parser that I used to debug this.#302kumpera wants to merge 6 commits into
kumpera wants to merge 6 commits into
Conversation
added 5 commits
April 21, 2021 09:22
…tor to be virtual.
Add a dtor to live_model_impl and flush the queues from it. Why flushing from live_model_impl is ok but from async_batcher is not? async_batcher holds un-owned references to a lot of stuff and the expectation is that those objects won't be valid during destruction. Flushing data in async_batcher requires those dangling pointers to be valid so we cannot do it from the dtor. Then why is it ok to do it from live_model_impl dtor? As it turns out, it holds owning references to pretty much everything that's needed for flushing. Did you say pretty much? Yes, there are a lot of un-owned references in live_model_impl as well but they look related to initialization only. Life is too short otherwise.
ataymano
reviewed
Apr 21, 2021
olgavrou
reviewed
Apr 22, 2021
olgavrou
reviewed
Apr 22, 2021
ataymano
approved these changes
Apr 23, 2021
Member
ataymano
left a comment
ataymano
left a comment
There was a problem hiding this comment.
LGTM. Although looks like there is some consistent issue with indentation (maybe spaces vs tabs?)
Contributor
Author
Yes, formatting is all bad because I used VS which I can't figure out how to make it respect our style. I'll fix it using clang-fmt before committing. |
olgavrou
approved these changes
Apr 26, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
parser.py can will now parse all preamble messages in a file.
rl.net.cli was not disposing its live model, now it does.
Introduced some basic logging to the batching process so we can do basic monitoring without having to a debugger or adding printf statements.
The extra logging turned out to be a problem because at-exit flushing was done from async_batch dtor and accessing a bunch of dangling pointers - fun stuff!
I fixed that by moving the flushing to live_model_impl, which owns everything so its dtor can do it safely.