-
Notifications
You must be signed in to change notification settings - Fork 344
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
perf: various improvements in pusher, pushsync, salud, reacher #4958
Conversation
case errors.Is(err, ErrShallowReceipt): | ||
fallthrough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it will go to default?
If this is ok, should we have a unit test for this case? both err==nil
and default
case are not covered by unit tests...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forwarder nodes no longer perform receipt checks, this is why the shallow receipt error check is removed.
see https://github.com/ethersphere/bee/pull/4958/files#diff-522faf538629254cbcb2fed372ba3f9c216497f868f45f6bd82cd6d5d6143c69R471
if err != nil { | ||
logger.Warning( | ||
"stamp with is no longer valid, skipping direct upload for chunk", | ||
ok, err := s.batchExist.Exists(op.Chunk.Stamp().BatchID()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If batch not found it will return nil? Is this also ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for direct uploads, the chunk resync is retried if this error is not nil. because batch is no longer valid at this point, we should stop syncing, hence the nil return
Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):