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

MM-52325: Bump morph #23394

Merged
merged 3 commits into from
May 24, 2023
Merged

MM-52325: Bump morph #23394

merged 3 commits into from
May 24, 2023

Conversation

agnivade
Copy link
Member

@agnivade agnivade commented May 16, 2023

This is needed to implement non-transactional migrations

https://mattermost.atlassian.net/browse/MM-52325

The Go MySQL driver has changed the `maxAllowedPacket` size from 4MiB to 64MiB. This is to make it consistent with the change in the server side default value from MySQL 5.7 to MySQL 8.0. If your `max_allowed_packet` setting is not 64MiB, then please update the MySQL config DSN with an additional param of `maxAllowedPacket` to match with the server side value. Alternatively, a value of 0 can be set to to automatically fetch the server side value, on every new connection, which has a performance overhead.

This is needed to implement non-transactional migrations

https://mattermost.atlassian.net/browse/MM-52325
```release-note
NONE
```
@agnivade agnivade added the 2: Dev Review Requires review by a developer label May 16, 2023
@agnivade agnivade requested a review from isacikgoz May 16, 2023 05:55
@mm-cloud-bot mm-cloud-bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 16, 2023
Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

LGTM, shall we add TestDatabaseSetFile/max_length as flaky test? Never seen it happened though but GH env seems to be more congested compared to previous one.

@agnivade
Copy link
Member Author

Yeah sure

@agnivade
Copy link
Member Author

Well actually, this seems like a CI env issue to me:

[mysql] 2023/05/16 06:12:41 packets.go:174: write tcp 172.18.0.10:60868->172.18.0.8:3306: write: broken pipe
2023-05-16T06:39:17.9088405Z     database_test.go:963: 
2023-05-16T06:39:17.9088886Z         	Error Trace:	/mattermost-server/mattermost-server/server/config/database_test.go:963
2023-05-16T06:39:17.9089168Z         	Error:      	Received unexpected error:
2023-05-16T06:39:17.9089425Z         	            	invalid connection
2023-05-16T06:39:17.9089773Z         	            	failed to update row for toolong

invalid connection usually means something went wrong with the DB.

@agnivade
Copy link
Member Author

No wait, that's an actual failure. I boiled it down to a mysql driver upgrade from 1.7.0 to 1.7.1: go-sql-driver/mysql@v1.7.0...v1.7.1. They bumped up the maxPacketSize from 4MB to 64MB. And that's what's causing this. Trying to see how to fix the test.

@agnivade
Copy link
Member Author

Ok I had to add a &maxAllowedPacket=4194304 to the DSN. This is possibly a breaking change as now the driver will send more packets by default. So customers may need to check their DB config and update the DSN to match with whatever is configured on the DB. I will add in the PR release notes.

@isacikgoz
Copy link
Member

Ok I had to add a &maxAllowedPacket=4194304 to the DSN. This is possibly a breaking change as now the driver will send more packets by default. So customers may need to check their DB config and update the DSN to match with whatever is configured on the DB. I will add in the PR release notes.

Thanks, yeah this may have some sort of performance impact right? Shall we do something around that?

@mm-cloud-bot mm-cloud-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 16, 2023
@agnivade
Copy link
Member Author

This only affects inserting text larger than 4MB, which I think only affects saving the config in DB. Don't believe we need a full blown performance test for this.

cc @amyblais - there is a potential breaking change with this PR which has happened because of a dependency upgrade (out of our control). I have updated the release notes accordingly for what customers should do.

@amyblais amyblais removed the 2: Dev Review Requires review by a developer label May 16, 2023
@agnivade
Copy link
Member Author

/update-branch

@agnivade agnivade added the 4: Reviews Complete All reviewers have approved the pull request label May 24, 2023
@agnivade agnivade merged commit 88d1743 into master May 24, 2023
33 checks passed
@agnivade agnivade deleted the bumpMorph branch May 24, 2023 10:23
@amyblais amyblais added Docs/Needed Requires documentation Changelog/Done Required changelog entry has been written labels May 25, 2023
@amyblais amyblais added this to the v8.0.0 milestone May 25, 2023
@amyblais amyblais removed the Docs/Needed Requires documentation label Jun 16, 2023
@amyblais amyblais added the Docs/Done Required documentation has been written label Jun 16, 2023
sbishel pushed a commit to sbishel/mattermost that referenced this pull request Aug 15, 2023
This is needed to implement non-transactional migrations

https://mattermost.atlassian.net/browse/MM-52325
```release-note
NONE
```

Co-authored-by: Mattermost Build <build@mattermost.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants