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

Chore: Startup Time #23210

Merged
merged 92 commits into from
Oct 19, 2021
Merged

Chore: Startup Time #23210

merged 92 commits into from
Oct 19, 2021

Conversation

ggazzo
Copy link
Member

@ggazzo ggazzo commented Sep 15, 2021

Proposed changes (including videos or screenshots)

The settings logic has been improved as a whole.

All the logic to get the data from the env var was confusing.

Setting default values was tricky to understand.

Every time the server booted, all settings were updated and callbacks were called 2x or more (horrible for environments with multiple instances and generating a turbulent startup).

Settings.get(......, callback); was deprecated. We now have better methods for each case.

Issue(s)

Steps to test or reproduce

Further comments

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2021

This pull request introduces 1 alert and fixes 1 when merging dd8f7b3 into 5443be7 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

fixed alerts:

  • 1 for Missing rate limiting

app/authentication/server/startup/index.js Outdated Show resolved Hide resolved
app/autotranslate/server/permissions.js Outdated Show resolved Hide resolved
app/channel-settings/server/startup.js Outdated Show resolved Hide resolved
app/discussion/server/permissions.js Outdated Show resolved Hide resolved
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

not sure what exactly is broken, but the process don't start up anymore.

@ggazzo ggazzo force-pushed the chore/startup-performance branch from a777524 to 0896860 Compare September 23, 2021 20:52
@RocketChat RocketChat deleted a comment from lgtm-com bot Sep 24, 2021
@ggazzo ggazzo requested a review from sampaiodiego October 15, 2021 03:48
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

  • ts is not being set
  • there is change on the previous behavior I'm not sure is good or bad.. Previously we couldn't change setting's value by only changing it on settings.add, but we could change everything else (like if a setting is public or not), but with the new implementation this is not possible anymore. looks like once the setting is saved on DB, it doesn't apply anything we change on settingsRegister.add
  • there a few errors during startup I think we should get rid of before merging, like:
Invalid setting code SMS_Mobex_from_number: Setting SMS_Mobex_from_number is of type int but got string

app/theme/server/server.js Outdated Show resolved Hide resolved
@ggazzo ggazzo merged commit ba15ba7 into develop Oct 19, 2021
@ggazzo ggazzo deleted the chore/startup-performance branch October 19, 2021 21:53
@ggazzo ggazzo restored the chore/startup-performance branch October 20, 2021 02:36
@ggazzo ggazzo deleted the chore/startup-performance branch October 20, 2021 02:40
@sampaiodiego sampaiodiego mentioned this pull request Oct 28, 2021
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.

2 participants