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

Adding more logs to OnRecovery payload #640

Merged
merged 8 commits into from
Mar 19, 2019

Conversation

vncoelho
Copy link
Member

@vncoelho vncoelho commented Mar 18, 2019

We need more logs for completely understanding OnRecoveryMessageReceived, otherwise, we can get confused if we are processing the payload from the RecoverPayload or we are receiving another payload from the network.

jsolman
jsolman previously approved these changes Mar 18, 2019
Copy link
Contributor

@jsolman jsolman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to understand whether the logs are during recovery, we need to include a log when recovery is finished. I agree with adding a single log for this purpose.

@vncoelho
Copy link
Member Author

I agree as well, simple and solves.
By checking the logs above the "$"{nameof(OnRecoveryMessageReceived)}: finished." we gonna be able to see information about the recovery.

@jsolman
Copy link
Contributor

jsolman commented Mar 18, 2019

Talked offline with @vncoelho Perhaps we can include some summary information in the finished log message for easier viewing.

@vncoelho
Copy link
Member Author

Genial, @jsolman. Looks quite informative as this.
Thanks for this increment!

Maybe I test this after #642, otherwise I need to merge it locally here because I am already testing that one.

Co-Authored-By: jsolman <jsolinsky@gmail.com>
erikzhang
erikzhang previously approved these changes Mar 19, 2019
}
finally
{
Log($"{nameof(OnRecoveryMessageReceived)}: finished (valid/total) " +
$"ChgView: {validChangeViews}/{totalChangeViews} " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total is not the count of CN?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is:
ChangeViewMessages = LastChangeViewPayloads.Where(p => p != null).Select(p => RecoveryMessage.ChangeViewPayloadCompact.FromPayload(p)).Take(this.M()).ToDictionary(p => (int)p.ValidatorIndex),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • total is the total number of messages examined during this run of the recovery
  • valid is the the number of those examined that were valid

@vncoelho
Copy link
Member Author

vncoelho commented Mar 19, 2019

@jsolman,

In general I am getting this log after "send recovery to resend commit"

[14:37:18.201] OnRecoveryMessageReceived: finished (valid/total) ChgView: 0/0 PrepReq: 0/0 PrepResp: 0/0Commits: 1/1

But in other cases, for example "[15:09:45.502] send recovery from view: 0 to view: 1" I got empty payloads.

[14:38:41.836] OnRecoveryMessageReceived: finished (valid/total) ChgView: 0/0 PrepReq: 0/0 PrepResp: 0/0Commits: 0/0
[14:38:41.836] OnRecoveryMessageReceived: height=1690 view=1 index=3
[14:38:41.836] OnRecoveryMessageReceived: finished (valid/total) ChgView: 0/0 PrepReq: 0/0 PrepResp: 0/0Commits: 0/0

@jsolman
Copy link
Contributor

jsolman commented Mar 19, 2019

@vncoelho If you are already committed, you will not even consider anything accept the commits, so that would be why you get the first log most likely. The log won't show the total for a message type in the case that it doesn't even examine a particular type of messages.

As for the empty payloads, that isn't really empty payloads; if your node is already committed it returns early, but will still hit the finally message and print what looks like empty payloads, but really it means it didn't examine any of the message types. If total is 0, it doesn't mean that there wasn't anything in the payload, it means that the total examined payloads were 0.

  • total is the total number of messages examined during this run of the recovery
  • valid is the the number of those examined that were valid

@vncoelho vncoelho merged commit b71db63 into master Mar 19, 2019
@vncoelho vncoelho deleted the adding-logs-onrecover-payload branch March 19, 2019 18:43
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
Add change address documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants