-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added Redis Cache support for worker reports #230
base: master
Are you sure you want to change the base?
Conversation
Feature supports Redis AUTH as well as an expiry time that defaults to twice the NEBULA_MANAGER_CHECK_IN_TIME.
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.
Thanks for the PR, there are a few minor changes needed but other then that and that documentation of said changes will be needed for it to be merged (in the doc repo) this looks really nice, for smaller scales redis will likely be considerably easier and cheaper then Kafka so this will be a nice feature to have.
Also I'm not in any position to demand unit-testing as this is a project I started before I became obsessed with testing everything so a lot of the original code is still lacking in that department but the kitchen sink but it will be a nice extra if you feel up to it.
Hello @naorlivne I have incorporated the changes you have requested. Please take a look and let me know what you think. |
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.
A couple of more things that popped up, mostly few mentions of kafka rather then redis and a bit cleaner way of passing the host IP envvar.
I removed the ip address get feature. I agree with you at this point its moot. In the use case we have, I wanted to force the user to add the IP address since we wanted to create a reporting tool where the IP address is required. The fqdn method sometimes does not resolve to the correct host name and provides the container id i think. |
Plus I have also updated the documentation |
@@ -67,7 +67,6 @@ def get_memory_usage(): | |||
print("error getting memory usage") | |||
os._exit(2) | |||
|
|||
|
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.
PEP8 requires 2 empty lines between functions so this shouldn't be removed
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.
There's only 1 thing left (PEP8 change so not to cause a regression) but in terms of the worker this LGTM to me otherwise.
However I do not see any PR to the optional reporter component repo to let it pull the reports from redis so I'm wondering how will the reports ends up getting queried as without it.
I'll review the documentation changes you made in the docs repo.
Oh youre right! I did not realize there is a backed integration of Kafka. Super cool! Honestly, nebula has really helped me a lot. So I am trying to give back in whatever way I can. I would be happy to make changes there. Give me a few days. I am swamped right now. Ditto for the document changes. |
Thanks for giving back, it's what FOSS is built on after all, If possible can you maybe also give a bit of info on your Nebula usecase? I'm always curious to see how people are using it. |
We are actually trying to build a decentralized analytics framework in our lab at Georgia Tech. We needed a way to push analytics to the edge and thats when I came across Nebula! |
Hi @paritoshpr , Wanted to see how is it going, both regarding this ticket and your overall Nebula use? |
Hey @naorlivne |
Fixes #
Proposed Changes