Skip to content
This repository has been archived by the owner on Nov 7, 2023. It is now read-only.

Support obtaining and resetting the message log #207

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

sergefdrv
Copy link
Contributor

This pull request extends the message log functionality required to generate ViewChange messages.

Sergey Fedorov added 2 commits June 17, 2021 19:41
It does not seem useful to have the internal lock as a named field
since the structure is not exported.

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
This change simplifies the code and makes it more efficient.

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
@sergefdrv sergefdrv requested a review from nhoriguchi as a code owner June 17, 2021 17:47
@nhoriguchi nhoriguchi self-assigned this Jun 18, 2021
@sergefdrv sergefdrv added the enhancement New feature or request label Jun 18, 2021
@sergefdrv sergefdrv added this to the v0.1.0 milestone Jun 18, 2021
@sergefdrv sergefdrv linked an issue Jun 18, 2021 that may be closed by this pull request
Copy link
Contributor

@nhoriguchi nhoriguchi left a comment

Choose a reason for hiding this comment

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

I left one nitpicking comment, but can't find any critical issues on the changes.

sync.RWMutex

// Value incremented on each reset
epoch uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

This field is used to detect Reset() calls during supplyMessages(). So naming this more directly like "reset" might be suitable? I think that the word "epoch" can be used to represent the similar concept as "view", so it might be confusing for someone familiar with other consensus protocols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to replace it with a channel.

Sergey Fedorov added 2 commits June 21, 2021 16:20
This method will be required to generate ViewChange message.

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
This method will allow to replace the content of the log with
ViewChange/NewView messages which embed the preceding messages.

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
@sergefdrv sergefdrv force-pushed the message-log-extension branch from 538c814 to c3ba51d Compare June 21, 2021 14:22
@sergefdrv
Copy link
Contributor Author

Summary of changes:

  • switched to using a channel for detecting log reset
  • improved testing of the new functionality

@sergefdrv sergefdrv requested a review from nhoriguchi June 21, 2021 14:25
Copy link
Contributor

@nhoriguchi nhoriguchi left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you.

@nhoriguchi nhoriguchi merged commit ce22fdb into hyperledger-labs:master Jun 22, 2021
@sergefdrv sergefdrv deleted the message-log-extension branch June 28, 2021 08:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ReqViewChange message handling
2 participants