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

Improve log warnings #2043

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@bachase
Copy link
Contributor

commented Mar 9, 2017

No description provided.

@bachase bachase requested review from mellery451 and wilsonianb Mar 9, 2017

@@ -942,6 +942,8 @@ static bool saveValidatedLedger (
JLOG (j.warn())
<< "Transaction in ledger " << seq
<< " affects no accounts";

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 9, 2017

Contributor

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.

This comment has been minimized.

Copy link
@bachase

bachase Mar 9, 2017

Author Contributor

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.

@wilsonianb
Copy link

left a comment

👍 not sure why Travis keeps timing out

@bachase bachase added the Passed label Mar 10, 2017

@bachase bachase force-pushed the bachase:ripd1440 branch from 0eed97e to f4c4a62 Mar 14, 2017

@bachase bachase changed the title Log non-account transaction in warning (RIPD-1440) Improve log warnings Mar 14, 2017

@bachase bachase removed the Passed label Mar 14, 2017

@bachase

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2017

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

This comment has been minimized.

Copy link
@mellery451

mellery451 Mar 14, 2017

Contributor

do you want to make the same change on L456, or is that not relevant?

This comment has been minimized.

Copy link
@bachase

bachase Mar 14, 2017

Author Contributor

Yes! 🤦‍♂️

@bachase bachase force-pushed the bachase:ripd1440 branch from f4c4a62 to 5a4a556 Mar 14, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Mar 14, 2017

Codecov Report

Merging #2043 into develop will decrease coverage by <.01%.
The diff coverage is 50%.

@@            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
Impacted Files Coverage Δ
src/ripple/overlay/impl/PeerImp.cpp 0% <0%> (ø) ⬆️
src/ripple/app/ledger/Ledger.cpp 81.42% <100%> (+0.05%) ⬆️
src/ripple/core/impl/JobQueue.cpp 86.11% <0%> (-0.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0df1b09...9e245a8. Read the comment docs.

@wilsonianb
Copy link

left a comment

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?

@@ -432,7 +432,6 @@ PeerImp::close()
}
}
}

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Mar 14, 2017

Is this deleted line intentional?

This comment has been minimized.

Copy link
@bachase

bachase Mar 15, 2017

Author Contributor

No.. double 🤦‍♂️

@bachase

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2017

I think warning is still appropriate for the fail case for both inbound and outbound connections since it represents abnormal termination. But I'm willing to be convinced otherwise.

Improve log warnings:
Log non-account transaction in warning (RIPD-1440)
Log warning on PeerImp::fail (RIPD-1444)

@bachase bachase force-pushed the bachase:ripd1440 branch from 4fd12dc to 9e245a8 Mar 20, 2017

@bachase bachase added the Passed label Mar 20, 2017

@bachase

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

Rebased on 0.60.0 and squashed

@wilsonianb
Copy link

left a comment

👍

@scottschurr scottschurr referenced this pull request Mar 21, 2017

Merged

Develop next #2059

@scottschurr

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2017

Merged to develop as c981eb8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.