-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve log warnings #2043
Improve log warnings #2043
Conversation
@@ -942,6 +942,8 @@ static bool saveValidatedLedger ( | |||
JLOG (j.warn()) | |||
<< "Transaction in ledger " << seq | |||
<< " affects no accounts"; |
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.
I guess if you don't actually need a distinct log record for this JSON, you could just add it to the line above with a "\n" since they are at the same level. Looks fine to me either way.
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.
True. I've seen other JSON dumps go on their own line in some other spots of the log, but I'm not sure if that is universal.
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.
👍 not sure why Travis keeps timing out
@wilsonianb @mellery451, I added another commit for a similarly simple increase in warning. Please take a quick peek when you get a chance. |
@@ -442,7 +442,7 @@ PeerImp::fail(std::string const& reason) | |||
shared_from_this(), reason)); | |||
if (socket_.is_open()) | |||
{ | |||
JLOG (journal_.debug()) << reason; | |||
JLOG (journal_.warn()) << reason; |
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.
do you want to make the same change on L456, or is that not relevant?
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.
Yes! 🤦♂️
Codecov Report
@@ Coverage Diff @@
## develop #2043 +/- ##
==========================================
- Coverage 67.7% 67.7% -0.01%
==========================================
Files 680 680
Lines 49219 49221 +2
==========================================
+ Hits 33323 33324 +1
- Misses 15896 15897 +1
Continue to review full report at Codecov.
|
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.
I noticed this comment
https://github.com/ripple/rippled/blob/develop/src/ripple/overlay/impl/PeerImp.cpp#L170-L173
Do that should also apply in PeerImp::fail
?
src/ripple/overlay/impl/PeerImp.cpp
Outdated
@@ -432,7 +432,6 @@ PeerImp::close() | |||
} | |||
} | |||
} | |||
|
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.
Is this deleted line intentional?
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.
No.. double 🤦♂️
I think warning is still appropriate for the |
Log non-account transaction in warning (RIPD-1440) Log warning on PeerImp::fail (RIPD-1444)
Rebased on 0.60.0 and squashed |
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.
👍
Merged to develop as c981eb8 |
No description provided.