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.
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
feat: new grpc call for subscribing alerts and low balance alert (#864) #2023
base: master
Are you sure you want to change the base?
feat: new grpc call for subscribing alerts and low balance alert (#864) #2023
Changes from 3 commits
b7a77f6
a58c1c6
3a890e0
0f4a1c5
032482f
6458e81
947ab1a
d2cff5c
6f3ee1c
9ccda04
8dcf5cf
f7b7aab
0f95df4
3469df2
25b3954
510ebd4
abc763f
18286f6
9705f1a
1965731
9d380b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 think I see the desire to convert to a string here, but I'm concernedthis will cause some issues using plain old JSON.stringify, which isn't guaranteed to give the same string for equivalent objects. It's also a bit expensive of a procedure, and uses up more memory in the map.
Doesn't have to be addressed in this PR since it's not that simple, but maybe it can be addressed at the same time as cleanup. I'm thinking it'd make sense to add a timestamp property to the
Alert
object, and then convert alert's to a string identifier like[type]-[channel_id]
rather than converting the entire object to a json string.For now I think add a TODO here and it's ok with me.
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.
Yes, you're right, alerts already have timestamps attached within BaseAlert, adding
TODO
there, but I guess we should prepare cleanup as soon as possible to avoid memory issues.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.
Maybe even within this PR, wdyt? @sangaman
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.
Up to you, it's not a trivial effort.