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

Revise bandwidth reduction page with cellular measurements #2476

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

kb2ma
Copy link
Contributor

@kb2ma kb2ma commented Jan 3, 2023

Emphasize bandwidth used per feature, and simplify. Also add the effect of NetworkManager connectivity check.

@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 3, 2023

@vipulgupta2048 , I'm happy for any comments but most interested in the overall structure of the revision at this point. I don't want to start tweaking before we're happy with the approach. Overall I tried to focus on actual bandwidth usage because I think that's most relevant.

@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 5, 2023

@vipulgupta2048 , tightened up the presentation format. I responded and resolved some comments. The others remain to implement. Are you happy with the overall flow now?

@vipulgupta2048
Copy link
Member

I am satisfied with my care the overall flow @kb2ma Go for it.

@kb2ma kb2ma force-pushed the revise_bandwidth_reduction branch 2 times, most recently from bea15d8 to 16626b0 Compare January 7, 2023 16:47
@kb2ma kb2ma marked this pull request as ready for review January 7, 2023 16:59
@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 7, 2023

Addressed all comments and refined text. Ready for review, @vipulgupta2048 .

@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 7, 2023

@rosswesleyporter , I'd appreciate your review here. See this file for a decent rendering.

@rosswesleyporter
Copy link
Contributor

First, this is great! Very useful. Very clear.

Suggest emphasizing the Supervisor version and the balenaOS version. From reading, it's not immediately obvious that there is a substantial benefit to using 2.107.10 & 14.4.4 or later. But if I understand correctly there is a big benefit. Could add a sentence above the table. Or perhaps put the versions in the table itself.

Above the table you may want to mention that the settings are reversible. For instance, the VPN can be turned back on. For some customers, it may sound scary to turn off the VPN.

This may not be important enough to include, but turning off Cloudlink turns off the ability to HUP.

In Device Logging, perhaps change "or balena system" to "or the balena system"

Again, this is great!

@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 9, 2023

Glad to hear the doc is clear and useful, @rosswesleyporter . I can emphasize cautions and required versions. Speaking of versions, I'm going to move this PR back to draft until there is a more recent release for RPi4. I manually added a fix that's in a later version of balenaOS than the version currently in the doc, 2.107.10.

@kb2ma kb2ma marked this pull request as draft January 9, 2023 01:46
auto-merge was automatically disabled January 9, 2023 01:46

Pull request was converted to draft

pages/reference/supervisor/bandwidth-reduction.md Outdated Show resolved Hide resolved
pages/reference/supervisor/bandwidth-reduction.md Outdated Show resolved Hide resolved
pages/reference/supervisor/bandwidth-reduction.md Outdated Show resolved Hide resolved
pages/reference/supervisor/bandwidth-reduction.md Outdated Show resolved Hide resolved
pages/reference/supervisor/bandwidth-reduction.md Outdated Show resolved Hide resolved
pages/reference/supervisor/bandwidth-reduction.md Outdated Show resolved Hide resolved
@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 18, 2023

Thanks for the improvements to the Side Effects section, @dfunckt . I agree that many of them read better and incorporated in the latest commit.

I did continue to start the explanatory text as a new sentence. I also added a colon ":" after the bold heading. I wanted the words in bold to refer to the feature back up to the table. I think trying to make a sentence begin with the heading deflects from that intent.

@kb2ma kb2ma marked this pull request as ready for review January 19, 2023 11:19
@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 21, 2023

When merge this PR, must follow up with user for #2126. Also must review patterns related to bandwidth.

@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 21, 2023

@vipulgupta2048 , this PR is ready for your detailed review.

Copy link
Member

@vipulgupta2048 vipulgupta2048 left a comment

Choose a reason for hiding this comment

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

Looking fine-o-fine. I just pushed a new commit containing some of the changes I felt that could be helpful. The only review I make is to reduce commits down to 1 or 2 ideally. Since most of the changes are about improving the content of the same only.
Feel free to self-certify after that

Change-type: patch
Signed-off-by: Ken Bannister <[email protected]>
@kb2ma
Copy link
Contributor Author

kb2ma commented Jan 26, 2023

@balena-ci I self-certify!

@balena-ci balena-ci merged commit 02d0fc5 into master Jan 26, 2023
@balena-ci balena-ci deleted the revise_bandwidth_reduction branch January 26, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants