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

refactor(core): serve language from notifications subgraph #4349

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

Conversation

thevaibhav-dixit
Copy link
Contributor

No description provided.

@thevaibhav-dixit thevaibhav-dixit marked this pull request as ready for review April 24, 2024 12:18
Copy link
Member

@nicolasburtey nicolasburtey left a comment

Choose a reason for hiding this comment

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

we should probably talk about this during daily dev. I agree with @dolcalmi that the idea of moving the language to the notification pod is not the best solution. And the downtime we had last week was a manifestation of the issue.

the logic was that there is only one use case for the language, but I think this is not correct:

there were 2 use case:

  • sending notification
  • serving the language for the mobile users (it felt like this use case was omitted)

now there are more use cases, like with the integration of GPT

this make the notification part of the runtime to serve most requests from the mobile app. this make it that the odds of something going wrong increase. as opposed to have the notification pods as a non mandatory part of the runtime for frequent non critical requests

@bodymindarts
Copy link
Member

we should probably talk about this during daily dev. I agree with @dolcalmi that the idea of moving the language to the notification pod is not the best solution. And the downtime we had last week was a manifestation of the issue.

the logic was that there is only one use case for the language, but I think this is not correct:

there were 2 use case:

  • sending notification
  • serving the language for the mobile users (it felt like this use case was omitted)

now there are more use cases, like with the integration of GPT

this make the notification part of the runtime to serve most requests from the mobile app. this make it that the odds of something going wrong increase. as opposed to have the notification pods as a non mandatory part of the runtime for frequent non critical requests

Agree that we should revisit this discussion - probably at ACE (don't think daily will have enough time as the topic is quite deep).
This PR is intended to improve resiliency in the short term pending the outcome of that discussion. But we can also hold off on merging until we have talked about it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants