-
Notifications
You must be signed in to change notification settings - Fork 1
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
Benchmarking; add unit tests to LLO plugin; fix large channel add not working #65
Benchmarking; add unit tests to LLO plugin; fix large channel add not working #65
Conversation
samsondav
commented
Jun 26, 2024
•
edited
Loading
edited
- Use pbufs instead of json
- Implement binary codec for Outcome and Observation
- ** Add unit tests **
3e3bc46
to
5de6e26
Compare
4b1e248
to
291e59e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a whole lot of context over this so approving to unblock. I took a pass over the changes and did my best to try to understand what's going on and it seems mostly fine.
int64 unixTimestampNanoseconds = 3; | ||
repeated uint32 removeChannelIDs = 4; | ||
map<uint32, LLOChannelDefinitionProto> updateChannelDefinitions = 5; | ||
map<uint32, bytes> streamValues = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use a repeated messages instead of maps here? A map limits you to the provided key and value types only, but a repeated message gives you the option of adding more fields later on. e.g. you already have LLOStreamIDAndValue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah not necessary because Observation doesn't need to be serialized deterministically
- Use pbufs instead of json for Outcome and Observation - Implement "Update" which does add and replace instead of only add - Add some unit tests - Fix various bugs and nil crashes - Increase max channels - Add Verbose logging option - Performance improvements
a5c208e
to
0cbd3e0
Compare