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

Report all refreshCheck() outcomes and entry gist #1932

Conversation

rousskov
Copy link
Contributor

@rousskov rousskov commented Nov 1, 2024

All other refreshCheck() return statements already have a debugs().
Reporting StoreEntry gist helps find the right calls in busy proxy logs.

All other refreshCheck() return statements already have a debugs().
Reporting StoreEntry gist helps find the right calls in busy proxy logs.
Copy link
Contributor Author

@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.

I spent 10+ extra minutes trying to confirm refreshCheck() outcome for a particular test and decided to save that time by reporting the last two "silent" return statements... At least on of these two outcomes can be correctly identified using existing earlier debugs() statements in refreshStaleness(), but those statements are in another function and are not necessarily the last debugs() in the log before refreshCheck() exits, making analysis more difficult than it should be.

P.S. This review annotates this PR without requesting any changes.

return STALE_EXPIRES;
}

if (sf.max)
if (sf.max) {
debugs(22, 3, "returning STALE_MAX_RULE");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some existing debugs() in this function already use this (correct) minimalist style to report outcomes.

Some debugs() also add "YES/NO prefixes that are rather misleading for a non-boolean function that is not called something like isFresh(). Some debugs() also try to explain why a particular outcome was reached by describing the last condition checked, but such reporting can easily mislead because all but the very first outcome depend on multiple preconditions. For the two return statements reported in this PR, just letting the developer know which exit path got triggered is the best strategy IMO.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Nov 1, 2024
@yadij yadij removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Nov 3, 2024
squid-anubis pushed a commit that referenced this pull request Nov 3, 2024
All other refreshCheck() return statements already have a debugs().
Reporting StoreEntry gist helps find the right calls in busy proxy logs.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Nov 3, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 3, 2024
kinkie pushed a commit to kinkie/squid that referenced this pull request Nov 4, 2024
All other refreshCheck() return statements already have a debugs().
Reporting StoreEntry gist helps find the right calls in busy proxy logs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants