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

fix: node start up to continue on bootstrapper error #2910

Merged
merged 2 commits into from Apr 27, 2022
Merged

Conversation

istae
Copy link
Member

@istae istae commented Apr 27, 2022

Checklist

  • I have read the coding guide
  • My change requires a documentation update and I have done it
  • I have added tests to cover my changes.

Description

Motivation and context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):


This change is Reviewable

@istae istae added the ready for review The PR is ready to be reviewed label Apr 27, 2022
pkg/node/node.go Outdated
if err != nil {
return nil, fmt.Errorf("bootstrapper failed to fetch batch state: %w", err)
logger.Errorf("bootstrapper failed to fetch batch state: %w", err)
Copy link
Contributor

@mrekucci mrekucci Apr 27, 2022

Choose a reason for hiding this comment

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

Replace %w with %v.

Copy link
Collaborator

@ldeffenb ldeffenb left a comment

Choose a reason for hiding this comment

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

Questioning "bootstrapper" as being incorrect/misleading?

@@ -479,9 +479,9 @@ func NewBee(addr string, publicKey *ecdsa.PublicKey, signer crypto.Signer, netwo
libp2pPrivateKey,
o,
)
logger.Infof("bootstrapper done, took %s", time.Since(start))
logger.Infof("bootstrapper took %s", time.Since(start))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be bootstrapper or BatchState initialization?

Copy link
Member

@agazso agazso Apr 28, 2022

Choose a reason for hiding this comment

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

I tend to agree that bootstrapper is hard to understand without context. My suggestion would be postage snapshot syncing took %s because that would be consistent how the feature is called in the config (use-postage-snapshot).

if err != nil {
return nil, fmt.Errorf("bootstrapper failed to fetch batch state: %w", err)
logger.Errorf("bootstrapper failed to fetch batch state: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto here.

@istae istae merged commit ec67ac0 into master Apr 27, 2022
@istae istae deleted the bootstrapper-error branch April 27, 2022 17:46
@acud acud added this to the 1.6.0 milestone May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants