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

feat(share/getter): add support for ErrNotFound in shrex getter implementation #2073

Closed
wants to merge 6 commits into from

Conversation

walldiss
Copy link
Member

Overview

Resolves #2037

@walldiss walldiss added area:shares Shares and samples kind:refactor Attached to refactoring PRs labels Apr 14, 2023
@walldiss walldiss self-assigned this Apr 14, 2023
Comment on lines -74 to -75

fmt.Println(car.HeaderSize(carReader.Header))
Copy link
Member Author

Choose a reason for hiding this comment

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

just a sneaky cleanup. Didn't want to make an extra PR for that

// validate will collect peer.ID into corresponding peer pool
func (m *Manager) validate(_ context.Context, peerID peer.ID, msg shrexsub.Notification) pubsub.ValidationResult {
// Validate will collect peer.ID into corresponding peer pool
func (m *Manager) Validate(_ context.Context, peerID peer.ID, msg shrexsub.Notification) pubsub.ValidationResult {
Copy link
Member Author

Choose a reason for hiding this comment

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

Exported to allow adding peers in tests. Not super clean solution, I'm open for other suggestions how to populate peer manager with test peers.

Copy link
Member

Choose a reason for hiding this comment

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

Fine to me, as long as we consider migrating over to messenger in short term, which will fix this

case p2p.ErrInvalidResponse:
case errors.Is(getErr, context.DeadlineExceeded),
errors.Is(getErr, context.Canceled):
case errors.Is(getErr, p2p.ErrUnavailable):
Copy link
Member

Choose a reason for hiding this comment

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

Why is it called unavailable?

Copy link
Member

Choose a reason for hiding this comment

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

We've had this discussion before - its unavailable because it means we don't have it at the moment (but may have it in a few seconds)

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've renamed it to ErrNotFound to make it consistent to other errs in out codebase.

distractedm1nd
distractedm1nd previously approved these changes Apr 14, 2023
@walldiss
Copy link
Member Author

For some reason this one doesn't track pushes to the source branch anymore. Closing in favour of new one #2074

@walldiss walldiss closed this Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:refactor Attached to refactoring PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(share/getter): add support for ErrNotFound in shrex getter implementation
3 participants