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

shared-state: provide compressed cgi-bin endpoints #911

Merged

Conversation

spiccinini
Copy link
Contributor

@spiccinini spiccinini commented Jan 16, 2022

Add compression support to the public synchronization endpoints so that the data is transmitted and received using gzip when gzip is part of the URL query-string.

Why? mainly to help shared-state syncing when there is a bad quality link (low signal, too crowded airspace, etc) reducing the amount of packets that have to be transmitted. Also this leads to reduced bandwidth usage.

This change in its own does not change the current behaviour (uncompressed) but allows to use this feature in the future. There are two main possible mechanisms with different backwards compatibility policy:

  1. Provide a backwards compatible mechanism that allows a node to know if the other node supports gzip or not and use the new endpoint in that case. There are many options to do this but all add complexity.
  2. Add it in a backward incompatible way but with an upgrade path: provide compression support in the next libremesh release, with compression unused when syncing, and then in a future release switch to "always" compress.

I am more inclined to do the 2nd option as the first option not only adds code complexity but also runtime overhead and/or runtime complexity. What do you think?

@codecov-commenter
Copy link

Codecov Report

Merging #911 (f4ee585) into master (95da863) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
- Coverage   76.41%   76.38%   -0.03%     
==========================================
  Files          47       47              
  Lines        3934     3934              
==========================================
- Hits         3006     3005       -1     
- Misses        928      929       +1     
Impacted Files Coverage Δ
...kages/lime-system/files/usr/lib/lua/lime/utils.lua 81.06% <0.00%> (-0.30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95da863...f4ee585. Read the comment docs.

@ilario
Copy link
Member

ilario commented Jan 16, 2022

Second option sounds good.
Shouldn't the original line be erased?
I mean, now you added lines, but you should remove the original code line doing the same in the uncompressed way?

@germanferrero
Copy link
Contributor

germanferrero commented Jan 16, 2022

I think we could choose 1st option with Accept and Content-Type http headers , and OPTIONS requests, as an standard mechanism

@spiccinini
Copy link
Contributor Author

I think we could choose 1st option with Accept and Content-Type http headers , and OPTIONS requests, as an standard mechanism

Do you see how without adding additional requests per request or maintaining a state per peer and adding at least one more request in the flow?

Copy link
Member

@G10h4ck G10h4ck left a comment

Choose a reason for hiding this comment

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

Seems a good idea to me, for the future I agree on following the libremesh tradition (so we keep it lightwight) providing upgrade path instead of retro-compatibility.

About the code do I understand well that you forgot to delete a couple of lines from the former code?

@germanferrero
Copy link
Contributor

I think we could choose 1st option with Accept and Content-Type http headers , and OPTIONS requests, as an standard mechanism

Do you see how without adding additional requests per request or maintaining a state per peer and adding at least one more request in the flow?

No, what I had in mind was using an additional OPTION request. Now that I understand the "upgrade path" proposal I also think that it's the best option.

Add compression support to the public synchronization endpoints so that
the data is transmitted and received using gzip when appending ?gzip to
the URL query-string.
@spiccinini spiccinini force-pushed the shared-state-compression-first-part branch from f4ee585 to 75b53d5 Compare March 4, 2022 17:38
@spiccinini spiccinini requested a review from G10h4ck March 4, 2022 17:44
Copy link
Contributor

@germanferrero germanferrero left a comment

Choose a reason for hiding this comment

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

Thanks @spiccinini !

@spiccinini
Copy link
Contributor Author

I've run the following test:

root@LiMe-abc000:/# cat /tmp/shared-state/data/babeld-hosts.json > /tmp/foo
root@LiMe-abc000:/# cat /tmp/shared-state/data/babeld-hosts.json | gzip > /tmp/foo.gzip


root@LiMe-abc000:/# uclient-fetch -q -O- --timeout=3 --post-file='/tmp/foo' 'http://localhost/cgi-bin/shared-state/spam'
root@LiMe-abc000:/# uclient-fetch -q -O- --timeout=3 --post-file='/tmp/foo.gzip' 'http://localhost/cgi-bin/shared-state/bar?gzip=1'

root@LiMe-abc000:/#  md5sum  /tmp/shared-state/data/babeld-hosts.json  /tmp/shared-state/data/bar.json  /tmp/shared-state/data/spam.json
e4b23afbbdcbed68539fa8003d6be29c  /tmp/shared-state/data/babeld-hosts.json
e4b23afbbdcbed68539fa8003d6be29c  /tmp/shared-state/data/bar.json
e4b23afbbdcbed68539fa8003d6be29c  /tmp/shared-state/data/spam.json

@spiccinini spiccinini requested a review from luandro March 4, 2022 20:54
@spiccinini
Copy link
Contributor Author

Hi @ilario , @G10h4ck I have resolved the issues on this, is there anything missing? thanks!

@ilario
Copy link
Member

ilario commented Mar 14, 2022

For me whatever works is ok :)
A general thought: The retrocompatibility is not a must, as the compatibility between different LibreMesh releases is not assured and not even tested. (is this right?)

@spiccinini
Copy link
Contributor Author

For me whatever works is ok :) A general thought: The retrocompatibility is not a must, as the compatibility between different LibreMesh releases is not assured and not even tested. (is this right?)

right but the idea is to minimize issues, even if it is not fully compatible if the shared db does not work while you are upgrading the mesh then it will be harder for the community. So for this at least for me it is important.

@spiccinini spiccinini merged commit 5377f5d into libremesh:master Apr 23, 2022
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.

None yet

5 participants