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 precision of quick_abort_pct calculation #1921

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vshailesh
Copy link

@vshailesh vshailesh commented Oct 22, 2024

Removed if statement that prevented Division By Zero operation.
This situation will not arise now, so the check is no longer needed.

And Changed the math expression in store_client.cc.

Previously, Squid calculated data transfer percentage
using integer arithmetic. Now, it uses floating-point arithmetic,
which slightly changes the behavior of quick_abort_pct.

Example:
Assume we’re receiving 367 bytes in total.
quick_abort_pct is set to 75 percent.

Case 1: Integer Arithmetic (previously used)
75 percent of 367 is 276.

Case 2: Floating-Point Arithmetic (more precise)
75 percent of 367 is 275.25.
Consider floating-point effects when configuring quick_abort_pct

@squid-prbot
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you for working on this ugly code! I support this change in principle (despite the fact that it replaces integer math with floating point math), but it needs a bit more work. Please see my change request for details.

src/store_client.cc Show resolved Hide resolved
@rousskov
Copy link
Contributor

@vshailesh, please also add your contact information to CONTRIBUTORS file (to pass upcoming source-maintenance-tests CI check).

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 22, 2024
@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 24, 2024
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you for removing the no-longer-necessary code.

Please adjust PR description to briefly document the effect of that removal (e.g., I would expect a slight change in behavior for some setting of quick_abort_pct and possibly other quick_abort_... configuration options). When writing that comment, think as a sysadmin configuring Squid or reviewing their existing configuration (rather than as a Squid developer).

FYI: PR description formatting requirements are available, but most description formatting violations arise from exceeding the 72 character line length limit. Anubis will remove M-failed-description PR label a few minutes after you fix description formatting. If you cannot format PR description to pass Anubis check, do not worry about it -- we will do it for you.

I also expect that the source maintenance CI check will fail. Please use the diff provided in CI check output (follow the Details link) to adjust your CONTRIBUTORS modifications.

@vshailesh vshailesh force-pushed the SQUID-rewrite-math-expression branch 2 times, most recently from d742055 to 0d4b315 Compare October 25, 2024 06:31
@vshailesh
Copy link
Author

I have made changes to CONTRIBUTORS as suggested in the diff of CI check. Please retrigger the pipeline.

@vshailesh vshailesh force-pushed the SQUID-rewrite-math-expression branch from 0d4b315 to 5239596 Compare October 25, 2024 06:58
@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 25, 2024
@rousskov
Copy link
Contributor

I have made changes to CONTRIBUTORS as suggested in the diff of CI check. Please retrigger the pipeline.

I removed unwanted new line change (that change is visible in git diff 6c33c64..5239596 CONTRIBUTORS -- no need to run CI tests!). GitHub tests are running now.

Merge branch 'master' into SQUID-rewrite-math-expression

Just FYI: We do not need to keep PR branches in sync with master unless there are merge conflicts or other specific problems that require such synchronization. By default, ignore GitHub suggestions to merge master into your PR branch.

Alex: Please adjust PR description to briefly document the effect of that removal (e.g., I would expect a slight change in behavior for some setting of quick_abort_pct and possibly other quick_abort_... configuration options). When writing that comment, think as a sysadmin configuring Squid or reviewing their existing configuration (rather than as a Squid developer).

AFAICT, the above request is the only thing left before this PR can be cleared for merging. Thank you!

@rousskov
Copy link
Contributor

Jenkins, it is OK to test this PR.

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 28, 2024
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

@vshailesh, thank you for providing an example of changed Squid behavior. Your example focuses on quick_abort_pct calculations precision. You were right to disclose that change, but there is an arguably bigger change that the current example overlooks: Please add a brief statement describing whether/how the change affects quick_abort_pct behavior for responses that are smaller than 100 bytes in total message size. There is currently a contradiction between removed if statement and Squid documentation that this PR should clarify:

  • On one hand, code removed by this PR clearly suggests that, prior to this PR, responses smaller than 100 bytes would continue to be fetched (as if quick_abort_pct was set to -1), but now quick_abort_pct is going to be honored for those responses.
  • However, quick_abort_pct documentation in src/cf.data.pre continues to say "The cache by default continues downloading aborted requests which are ... less than 16 KB remaining". I do not see any relevant CheckQuickAbortIsReasonable() code, but perhaps that 16 KB check is elsewhere.

If Squid documentation is correct, then let's explicitly say (in the PR description) that the removed if statement had no effect on quick_abort_pct behavior (for such-and-such reasons). Otherwise, please remove that wrong statement from cf.data.pre and add a brief statement describing whether/how the change affects quick_abort_pct behavior for responses known to be smaller than 100 bytes. Needless to say, you should test to confirm these PR adjustments, whatever they are.

Once all the basic statements are in, I can help polish PR title/description, including its formatting, but I would prefer to rely on you to gather, describe, and validate the basic facts...

@@ -470,6 +470,7 @@ Thank you!
Sergey Merzlikin <sm@smsoft.ru>
Sergio Durigan Junior <sergiodj@sergiodj.net>
Sergio Rabellino <rabellino@di.unito.it>
Shailesh Vashishth <22538656+vshailesh@users.noreply.github.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

This entry is just fine if you like it, but if you prefer to use some other email address, please note that you can adjust this entry accordingly. Our automation is "smart" enough to allow you to pick the email address you prefer...

@yadij yadij changed the title changed the math expression in store_client.cc file Update precision of quick_abort_pct calculation Oct 28, 2024
@yadij yadij added the feature maintainer needs documentation updates for merge label Oct 28, 2024
@yadij
Copy link
Contributor

yadij commented Oct 28, 2024

Since this changes the behaviour of the Squid UI (config setting) this will also need a release notes mention for the quick_abort_pct directive.

@rousskov
Copy link
Contributor

Amos: Since this changes the behaviour of the Squid UI (config setting) this will also need a release notes mention for the quick_abort_pct directive.

@vshailesh, I can help with updating release notes once PR description has the necessary disclosures as detailed in my earlier change request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature maintainer needs documentation updates for merge M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants