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

Code review remediations #6

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Code review remediations #6

wants to merge 6 commits into from

Conversation

krokicki
Copy link
Member

I am addressing the following suggestions from the code review:

  1. Michael: what does the comment on line 127 mean? - removed confusing code/comment and moved logic to separate function that can be reused
  2. Anirudh: the embedded_model function is too long and not very cohesive - moved async queuing logic to a separate function, focusing the endpoint on HTTP concerns
  3. Stuart: the session dict should get cleaned up or it will leak memory over time - added clean up step to stop memory leak
  4. Mark: the session id should not be a UUID, it would be less convoluted and more efficient to use an integer counter - after some discussion, I decided to use a uuid4 instead of the uuid1. Since it is random it is more resistant to attacks and also makes it easier to identify different clients at a glance.

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.

1 participant