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 block height check into orderer bootstrap #2594

Closed

Conversation

Param-S
Copy link
Contributor

@Param-S Param-S commented May 17, 2021

Type of change

  • Bug fix

Description

  1. Channel with empty ledger led to crash in 2.2 release
    Higher version added a check to skip the channels with
    empty ledger & continue to start up. The same changes
    backported.

  2. Updated GetBlock construct to retrieve the block by number
    rather than using Iterator.

Signed-off-by: Parameswaran Selvam parselva@in.ibm.com

@Param-S Param-S requested a review from a team as a code owner May 17, 2021 12:27
@Param-S Param-S force-pushed the skpemptylgrordererboot branch from f11e603 to a10f12c Compare May 17, 2021 12:29
@Param-S Param-S changed the title Adding block height check into orderer bootstrap WIP: Adding block height check into orderer bootstrap May 17, 2021
@yacovm
Copy link
Contributor

yacovm commented May 18, 2021

Shouldn't we fix instead the core issue which is that the ledger creation for a channel isn't atomic?

@Param-S Param-S marked this pull request as draft May 24, 2021 09:49
@Param-S Param-S force-pushed the skpemptylgrordererboot branch 4 times, most recently from b310e5e to 733a542 Compare May 26, 2021 07:42
@Param-S
Copy link
Contributor Author

Param-S commented May 27, 2021

/ci-run

@github-actions
Copy link

AZP build triggered!

@Param-S Param-S force-pushed the skpemptylgrordererboot branch from 733a542 to 3bc096e Compare May 27, 2021 15:40
@Param-S Param-S marked this pull request as ready for review May 27, 2021 17:35
@Param-S Param-S changed the title WIP: Adding block height check into orderer bootstrap Adding block height check into orderer bootstrap May 27, 2021
@@ -113,16 +113,18 @@ type Registrar struct {
// ConfigBlock retrieves the last configuration block from the given ledger.
// Panics on failure.
func ConfigBlock(reader blockledger.Reader) *cb.Block {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a UT for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will update the PR with UT

Copy link
Contributor

Choose a reason for hiding this comment

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

The main intention of adding the UT was to cover the panic paths. Given that this function can take a mock as input, it should be straight forward to write this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, as you may want to make these changes into master as well. I'll suggest to brake these changes into two. One the original backporting PR as is and new changes in the master first.

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 corrected the UT now covering all error scenarios. Please check.

Should i need to create a separate PR for master branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2635 created a separate PR for main

Channel with empty ledger led to crash in 2.2 release
Higher version added a check to skip the channels with
empty ledger & continue to start up. The same changes
backported.

Signed-off-by: Parameswaran Selvam <parselva@in.ibm.com>
@Param-S
Copy link
Contributor Author

Param-S commented Jun 8, 2021

Opened different PRs. Closing this one

@Param-S Param-S closed this Jun 8, 2021
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.

3 participants