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

Feature/change redis struct #97

Open
wants to merge 54 commits into
base: production
Choose a base branch
from

Conversation

MahdiPresario001
Copy link
Collaborator

@MahdiPresario001 MahdiPresario001 commented Nov 4, 2024

  • Responses not longer go through redis pubsub, but a response queue is created for each query, with a TTL
  • Contenders selection for organic queries:
  • Only keep contenders who make less than 1% 500s

  • Take the number of REQUESTS_429 in consideration too for selection

  • switch from redis to hiredis for better performance
  • change factory config function to use a global var
  • passing psqldb connection instead of the entire psql db variable to psql functions (contenders selection mainly)

docker-compose.yml Outdated Show resolved Hide resolved

_config = None

async def factory_config() -> Config:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only issue with this is the cache is not configurable now.

Is there any benefit over the other solution? looks to be doing the exact same thing but with two funcs and global var as opposed to one function

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the psql connection errors were solved by this. was proposed by @narigama
it's cleaner this way, we only create the config once, no caching needed (which was causing many issues)

@namoray
Copy link
Collaborator

namoray commented Nov 5, 2024

LGTM apart from the cache stuff, not sure if/why we need to do that

@MahdiPresario001 MahdiPresario001 force-pushed the feature/change-redis-struct branch from d98c4b0 to 2357fcd Compare November 5, 2024 21:28
@MahdiPresario001 MahdiPresario001 force-pushed the feature/change-redis-struct branch from 8e1fffe to d51d702 Compare November 8, 2024 00:27
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.

3 participants