-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: move non-relevant logs to debug
log level
#87
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
WalkthroughThe changes involve updates to error handling, logging levels, and method signatures in the Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go.work.sum
- tests/go.mod
- tests/go.sum
Files selected for processing (6)
- beanstalkjobs/connection.go (6 hunks)
- beanstalkjobs/item.go (1 hunks)
- beanstalkjobs/listen.go (2 hunks)
- tests/configs/.rr-beanstalk-init.yaml (1 hunks)
- tests/jobs_beanstalk_durability_test.go (1 hunks)
- tests/jobs_beanstalk_test.go (5 hunks)
Files skipped from review due to trivial changes (1)
- beanstalkjobs/item.go
Additional comments: 14
beanstalkjobs/connection.go (5)
17-17: The introduction of
errBeanstalkTimeout
is consistent with the PR's objective to standardize error handling and reduce verbosity in logs.98-101: The change from
errors.Errorf
tostderr.Join
in theReserve
method is a refinement that aligns with the PR's goal to make error handling less verbose.129-129: The
Stats
method now accepts a context as an argument, which is a change in the method signature. This change should be verified across the codebase to ensure that all calls to this method have been updated accordingly.
The verification process did not find any instances of the old or new method signatures for
Stats
in the codebase. This could mean that the method is not used elsewhere or that the search was not comprehensive enough to find the usage. However, based on the information provided, there are no indications of issues with the change to theStats
method signature.Final review comment:
The change to the
Stats
method signature to include a context parameter has been made, and verification did not reveal any instances of the old or new signatures, suggesting that there may not be any direct usage of this method in the codebase or that the search was not exhaustive. It is recommended to ensure that all calls to this method, if any, are updated to match the new signature.
- 238-246: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [210-245]
The refactoring of the
checkAndRedial
method with a switch statement and the handling of specific error conditions is an improvement that makes the error handling logic clearer and more maintainable.
- 243-243: The handling of the
errBeanstalkTimeout
error in thecheckAndRedial
method is a good use of the newly introduced variable and aligns with the PR's objective to standardize error handling.beanstalkjobs/listen.go (1)
- 20-31: The changes in error handling align with the PR's objective to reduce log verbosity by handling
errBeanstalkTimeout
more succinctly and continuing the loop without logging it as an error. This should make the logs less verbose by not logging expected timeout conditions as errors.tests/configs/.rr-beanstalk-init.yaml (1)
- 13-18: The change in the logging level from
info
todebug
is consistent with the PR's objective to reduce log verbosity and is correctly implemented in the configuration file.tests/jobs_beanstalk_durability_test.go (1)
- 39-39: The version number has been updated to "2023.3.0", which should reflect the new version of the configuration being tested. Ensure that this version is consistent with the intended test environment and that any related documentation or dependencies are also updated accordingly.
tests/jobs_beanstalk_test.go (6)
41-58: The
GracefulShutdownTimeout
has been significantly reduced from 60 seconds to 2 seconds in theTestBeanstalkInit
function. This change could potentially affect the behavior of the system during shutdown in test scenarios. Ensure that this new timeout value is sufficient for all resources to be released properly during the tests.44-47: The version number in the configuration has been updated from "2.9.0" to "2023.3.0". This change should be reflected in any documentation or other test cases that rely on the version number for consistency.
381-387: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the
TestBeanstalkStats
function. Ensure that this change is consistent with the intended versioning strategy and that all references to the version number are updated accordingly.498-504: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the
TestBeanstalkDeclare
function. Verify that this version update is accurately represented in all related documentation and configuration files.578-584: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the
TestBeanstalkJobsError
function. Confirm that this version change is properly synchronized across all test cases and documentation where the version number is mentioned.659-665: The version number in the configuration has been updated from "2.9.0" to "2023.3.0" in the
TestBeanstalkNoGlobalSection
function. As with the other test functions, ensure that this version update is consistent with the project's versioning policy and that all references to the version number are updated.
@@ -118,13 +120,13 @@ func (cp *ConnPool) Delete(_ context.Context, id uint64) error { | |||
return errors.Errorf("err: %s\nerr redial: %s", err, errN) |
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.
The use of errors.Errorf
in the Delete
method seems inconsistent with the PR's objective to reduce verbosity. Consider using stderr.Join
or a similar approach to align with the changes made in the Reserve
method.
Signed-off-by: Valery Piashchynski <[email protected]>
debug
log level
Reason for This PR
closes: roadrunner-server/roadrunner#1804
Description of Changes
debug
log level fromwarn
.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]
git commit -s
).CHANGELOG.md
.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests