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

Split up Server into Renderer/API for Staging #9827

Merged
merged 22 commits into from
Sep 10, 2024

Conversation

FinnIckler
Copy link
Member

Recreated so I can deploy from it

@FinnIckler FinnIckler added deploy-staging Should be deployed to staging and removed deploy-staging Should be deployed to staging labels Aug 29, 2024
@FinnIckler FinnIckler removed the deploy-staging Should be deployed to staging label Aug 29, 2024
@FinnIckler FinnIckler added the deploy-staging Should be deployed to staging label Aug 29, 2024
@FinnIckler FinnIckler removed the deploy-staging Should be deployed to staging label Aug 29, 2024
@FinnIckler FinnIckler added deploy-staging Should be deployed to staging and removed deploy-staging Should be deployed to staging labels Aug 29, 2024
@FinnIckler
Copy link
Member Author

FinnIckler commented Aug 29, 2024

API server is still using Webbrick for some reason (maybe puma has to be in the gem file and not just installed by doing gem install puma in the dockerfile 😄 ), but I think everything else works? I'll check the routes tomorrow

@FinnIckler FinnIckler marked this pull request as ready for review August 29, 2024 18:14
@FinnIckler
Copy link
Member Author

FinnIckler commented Aug 29, 2024

mh.. Ok I did a little bit of testing and it looks like we do need sessions because we use sessions in APIs all the time to check for current_user. Mainly stuff that's related to the user like /me/token or permissions

@FinnIckler FinnIckler added the deploy-staging Should be deployed to staging label Aug 30, 2024
@FinnIckler FinnIckler removed the deploy-staging Should be deployed to staging label Aug 30, 2024
Copy link
Contributor

@dunkOnIT dunkOnIT left a comment

Choose a reason for hiding this comment

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

Per discussion in Slack call

@gregorbg
Copy link
Member

As far as I can see, this PR does nothing to set WEB_CONCURRENCY on the individual containers. So every service that is spun up will run 8 workers?! Or am I overlooking something in the diff?

@FinnIckler
Copy link
Member Author

FinnIckler commented Sep 10, 2024

no. We are still setting web concurrency here worker_count = Integer(ENV.fetch("WEB_CONCURRENCY") { Concurrent.available_processor_count }), meaning staging will run 2 workers + 1 API worker

@FinnIckler
Copy link
Member Author

The next PR will be the "set everything to 1 cpu and disable web concurrency PR". It's also important to say that this is staging only so we can run the load test with this setup

@gregorbg
Copy link
Member

Ah, okay. So instead of setting the env variable to 1, you're simply provisioning the infrastructure to only even provide one core in the first place.

If you have this on your radar as a follow-up, then that's totally fine by me.

@FinnIckler FinnIckler merged commit 19d74c5 into main Sep 10, 2024
2 checks passed
@FinnIckler FinnIckler deleted the feature/split-up-api-staging branch September 10, 2024 11:40
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