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

Update SqlSettings.MaxIdleConns default setting #7063

Merged
merged 6 commits into from
Apr 22, 2024
Merged

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Apr 12, 2024

Summary

Per https://github.com/mattermost/mattermost/blob/98712737e600e8b11e7d72ea90faa266227aa855/server/public/model/config.go#L1236-L1238.

It seems like the default setting was always at 20, and the docs were always incorrect.

Ticket Link

None

@hanzei hanzei added the 1: Dev Review Requires review by a core commiter label Apr 12, 2024
@hanzei hanzei requested a review from agnivade April 12, 2024 09:54
Copy link

Newest code from hanzei has been published to preview environment for Git SHA c8594b6

Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Long term, this would be a pain to maintain. Because it is completely manual and there's no sync with the code. I'd rather just have a link the the default config.json generated for any config setting.

@hanzei hanzei requested a review from cwarnermm April 13, 2024 07:32
@hanzei hanzei added 2: Editor Review Requires review by an editor and removed 1: Dev Review Requires review by a core commiter labels Apr 13, 2024
@cwarnermm
Copy link
Member

@hanzei - Please update the default value in the table to match the search metadata. Thanks!

@cwarnermm
Copy link
Member

@agnivade "Because it is completely manual and there's no sync with the code. I'd rather just have a link the the default config.json generated for any config setting." -- I agree. It would be ideal to find a way to generate config docs directly from code rather than maintain the docs manually. Lowers the potential for errors, and reduces manual effort each release.

Copy link

Newest code from cwarnermm has been published to preview environment for Git SHA fd85900

@agnivade
Copy link
Member

It is not impossible. For our load-test repo, we can auto-generate the default config from struct fields which can be easily exposed in the Go pkg documentation at https://pkg.go.dev/github.com/mattermost/mattermost/server/public@v0.1.0/model#Config.

This is something @isacikgoz created. But we haven't ported it to the main MM repo yet.

Copy link

Newest code from cwarnermm has been published to preview environment for Git SHA 2fa290a

Copy link

Newest code from hanzei has been published to preview environment for Git SHA 7086644

Copy link

Newest code from hanzei has been published to preview environment for Git SHA e76ef22

@hanzei
Copy link
Contributor Author

hanzei commented Apr 22, 2024

@cwarnermm Good catch; I also updated the table. PTAL.

@cwarnermm cwarnermm added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Editor Review Requires review by an editor labels Apr 22, 2024
@cwarnermm cwarnermm merged commit 8bb9b18 into master Apr 22, 2024
5 checks passed
@cwarnermm cwarnermm deleted the hanzei-patch-1 branch April 22, 2024 15:08
Copy link

Newest code from cwarnermm has been published to preview environment for Git SHA db901f6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants