Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

paritoshpr
Copy link

Fixes #

Proposed Changes

  • Supports Redis as an alternate and in addition to Kafka for acquiring worker logs
  • Supports cache expiry time with a default of twice the manager_check_in_time
  • Supports Redis AUTH mechanism as well

Ramanan added 3 commits September 10, 2021 12:52
Feature supports Redis AUTH as well as an expiry time that defaults to twice the NEBULA_MANAGER_CHECK_IN_TIME.
functions/misc/server.py Outdated Show resolved Hide resolved
worker.py Show resolved Hide resolved
Copy link
Member

@naorlivne naorlivne left a 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.

@paritoshpr
Copy link
Author

Hello @naorlivne I have incorporated the changes you have requested. Please take a look and let me know what you think.

functions/misc/server.py Outdated Show resolved Hide resolved
functions/misc/server.py Outdated Show resolved Hide resolved
worker.py Outdated Show resolved Hide resolved
worker.py Outdated Show resolved Hide resolved
Copy link
Member

@naorlivne naorlivne left a 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.

@paritoshpr
Copy link
Author

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.

@paritoshpr
Copy link
Author

Plus I have also updated the documentation

@@ -67,7 +67,6 @@ def get_memory_usage():
print("error getting memory usage")
os._exit(2)


Copy link
Member

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

Copy link
Member

@naorlivne naorlivne left a 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.

@paritoshpr
Copy link
Author

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.

@naorlivne
Copy link
Member

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.

@paritoshpr
Copy link
Author

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!

@naorlivne
Copy link
Member

Hi @paritoshpr ,

Wanted to see how is it going, both regarding this ticket and your overall Nebula use?

@paritoshpr
Copy link
Author

Hey @naorlivne
Apologies. I havent been able to get to these fixes. Hopefully I'll be able to squeeze some time at the end of the month. Apart from that Nebula has been running great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants