-
Notifications
You must be signed in to change notification settings - Fork 15
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
Improve suma authentication concurrency #2531
Conversation
2a0bcd0
to
c5a4d30
Compare
c5a4d30
to
12a60cc
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 brought this up during a chat with @nelsonkopliku earlier today, but I wonder if it would be interesting at some point to have something like Cachex available to enable caching of certain frequently accessed state ? We would also get a nice interface for TTL, expiry/refresh, warming etc. This would also mean not having to rely on a GenServer for frequent state reads from different execution contexts (I'm not fully sure if that is the case with the state of the GenServer here, though).
Thank you for you comment @gagandeepb |
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.
Hey @arbulu89 thanks for this!
Besides some cosmetics my main comment is about keeping what you have extracted as authenticated = setup()
inside the auth flow.
12a60cc
to
7fcfbb2
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.
LGTM @arbulu89, thanks!
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.
Great work! I really like this approach 👍
Description
Change the SUMA interaction module to use the GenServer only to handle the authentication, not the whole sequence of queries. This way, only the authentication is a "locked" process, and all the other requests can happen concurrently.
The other "small" improvement is that the when we do a "batch" discovery (on saving SUMA settings for instance), we just authenticate once, and use that data for the rest. Otherwise, as the auth is still sequential, it would've been tried for each individual host, and when the user/password are wrong for instance, this could take a lot (5-10 seconds per host).
It is just an improvement of performance in reality, nothing else.
As things happen in the background the user might not even notice things (except, some toast are happening faster, etc).
How was this tested?
Unit tests updated, and I have tested manually with our own SUMA instance.
Next:
The
discovery
when we save the settings must happen async, so thepatch
is not holded there for a while (which can even create timeout issues)