-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Changes cache hosts warning / critical levels and continue on multiple hosts #18852
Changes cache hosts warning / critical levels and continue on multiple hosts #18852
Conversation
Hi @wiardvanrij. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
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.
Please see my propositions on the possible contribution improvement
Hi @wiardvanrij thank you for this useful contribution. I have an improvement proposition for this PR, that I described in the review comments. Please let me know what you think! |
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.
Thanks for the refactoring, please see the remaining comment about a couple of minor changes required
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.
Few final cosmetic changes required
$loggerMessage = implode(" ", $unresponsiveServerError); | ||
|
||
if ($errorCount == count($servers)) { | ||
$this->logger->critical('No cache server(s) could be purged ' . $loggerMessage, compact('server', |
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.
Each parameter should be in line by itself in multiline function call, example:
$this->logger->critical(
'No cache server(s) could be purged ' . $loggerMessage,
compact('server', 'formattedTagsChunk')
);
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.
Sorry I think I misunderstood you :) It is changed now
Hi @wiardvanrij, thank you for your contribution! |
…ue on multiple hosts #18852
Description (*)
When purging varnish hosts it could be possible 1 or more hosts are unresponsive.
Two changes here:
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)