-
Notifications
You must be signed in to change notification settings - Fork 62
gzip notify/listen payloads #311
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
Conversation
|
All tests passed but I'm not sure if they cover the postgres notify/listen system completely. I can expand the steps to try to check those payloads if necessary. |
|
The postgres notify/listen system is tested in the feature tests. They spin up a database and actually do webrtc handshakes etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds gzip compression to Postgres NOTIFY/LISTEN payloads to address the 8KB size limit that can be hit on certain networks. The implementation introduces compression at the publish side and decompression at the listen side.
Key Changes:
- New gzip utility functions for compressing and decompressing byte arrays
- Modified Postgres NOTIFY/LISTEN to compress payloads before base64 encoding and decompress after decoding
- Removed debug logging code for a specific Poki topic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/util/gzip.go | Adds new utility functions GzipCompress and GzipDecompress using gzip.BestSpeed compression level |
| internal/signaling/stores/postgres.go | Integrates gzip compression/decompression into the publish and listen flow, removes debug logging for oversized payloads |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Maybe this is the time to add a |
|
A payloads table sounds a bit overkill for now. This is just for a couple of times when the webrtc descriptor object is larger than 8kb. gzipping with bestspeed is super fast and probably makes the whole thing faster in the end. |
|
Fair. |
|
We already base64 any payload at the moment (here). I guess that's one of the issues why that webrtc descriptor message gets too big. So doing a quick gzip before the base64 is not a bad idea. |
I know we already did that. And gzip is a good idea. |
|
The webrtc descriptor can apparently be very big in certain network conditions. Poki was trying this during an event in a public space and ran into this issue. Judging from the logs it happens once in a while, not often. |
cd3c884 to
0b54246
Compare
This reverts commit ac86d2d.
Gzip the postgres NOTIFY/LISTEN payloads. This is mostly about the 8kb limit that we hit sometimes depending on the network you are connected to, but other payloads can also benefit from it.