Added Redis Cache support for worker reports#230
Conversation
Feature supports Redis AUTH as well as an expiry time that defaults to twice the NEBULA_MANAGER_CHECK_IN_TIME.
naorlivne
left a comment
There was a problem hiding this comment.
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. |
naorlivne
left a comment
There was a problem hiding this comment.
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 |
| print("error getting memory usage") | ||
| os._exit(2) | ||
|
|
||
|
|
There was a problem hiding this comment.
PEP8 requires 2 empty lines between functions so this shouldn't be removed
naorlivne
left a comment
There was a problem hiding this comment.
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