Turn ChangeString into a PeerChange enum #263
Conversation
| if change_action == ChangeAction::Modified | ||
| && diff | ||
| .changes() | ||
| .iter() | ||
| .all(|c| *c == PeerChange::NatTraverseReattempt) | ||
| { | ||
| // If this peer was "modified" but the only change is a NAT Traversal Reattempt, | ||
| // don't bother printing this peer. | ||
| return; | ||
| } |
There was a problem hiding this comment.
This was what resulted in lots of "peer was modified" messages, because the NatTraverseReattempt "change" was somewhat of a hack to trigger NAT traversal attempts for that peer.
It might actually make more sense to remove this PeerChange::NatTraverseReattempt "change" (which again, isn't actually a change) and instead just collect all unconnected peers that have endpoint candidates and try to connect to them after calls to fetch.
That would probably remove a decent amount of special-cased code.
There was a problem hiding this comment.
+1 for removing PeerChange::NatTraverseReattempt in some future PR.
| ); | ||
|
|
||
| for change in diff.changes() { | ||
| log::debug!(" {}", change); | ||
| if let PeerChange::Endpoint { .. } = change { |
There was a problem hiding this comment.
I felt that endpoint changes merited an info print instead of debug. When I see that a peer was modified I typically want to know if they got a sensible endpoint.
There was a problem hiding this comment.
Makes good sense. If we want to prevent micro-duplication, we could do something like:
let level = if matches!(update, PeerChange::Endpoint { .. }) { Level::Info } else { Level::Debug };
log::log(level, " {}", change);
There was a problem hiding this comment.
@bschwind just to understand this better before I fully review: did #262 remove all the spurious "peer modified" messages, or not yet and this fix is needed to make the logs "completely nice"?
Turning ChangeString into less dynamic enum makes good sense to me, I just need to think more about the functional changes.
No, there were still spurious "peer modified" messages because it counts a "NAT traversal reattempt" as a modification of the peer. |
|
Nice, so much cleaner. I didn't look deeper than this diff but I definitely agree if "NAT Traverse Reattempt" is all that's adding these extra lines and not actually producing any useful information to the user, we should just get rid of it. |
…eattempt as a modification
bschwind
force-pushed
the
change-string-enum
branch
from
3bacbb5 to
6222d5d
Compare
|
Rebased on the latest |
| ); | ||
|
|
||
| for change in diff.changes() { | ||
| log::debug!(" {}", change); | ||
| if let PeerChange::Endpoint { .. } = change { |
There was a problem hiding this comment.
Makes good sense. If we want to prevent micro-duplication, we could do something like:
let level = if matches!(update, PeerChange::Endpoint { .. }) { Level::Info } else { Level::Debug };
log::log(level, " {}", change);| if change_action == ChangeAction::Modified | ||
| && diff | ||
| .changes() | ||
| .iter() | ||
| .all(|c| *c == PeerChange::NatTraverseReattempt) | ||
| { | ||
| // If this peer was "modified" but the only change is a NAT Traversal Reattempt, | ||
| // don't bother printing this peer. | ||
| return; | ||
| } |
There was a problem hiding this comment.
+1 for removing PeerChange::NatTraverseReattempt in some future PR.
...So that we don't print NAT traversal reattempts as a "modification" on the peer. This should reduce "Peer X was modified" messages which can occur over and over on subsequent calls to
innernet fetchCloses #261