-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add expected CID param to AddPiece #7175
Conversation
9ebc95d
to
1af8d96
Compare
1af8d96
to
1916a78
Compare
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.
Logic looks good, just making sure this isn't breaking the v0 API.
@@ -167,6 +167,7 @@ Inputs: | |||
}, | |||
null, | |||
1024, | |||
null, |
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.
This shouldn't change. Do we need to stub out a version of the v0 API that passes nil?
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.
This is the worker API which is mostly internal, and generally ok to change
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.
Yeah... but is there any real reason to? Alternatively, if it's internal, is there any way to make sure it's always in the "unstable" API, and never gets stabilized?
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.
(doesn't really block merge)
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.
I'd call this API 'internal'.
That said, changing it does require miners to update all their workers, which can be quite inconvenient.. I maybe see a way to make this work without changing this API
Flaky test disabled in #7176. |
Codecov Report
@@ Coverage Diff @@
## master #7175 +/- ##
==========================================
+ Coverage 30.29% 34.82% +4.52%
==========================================
Files 677 684 +7
Lines 79752 80153 +401
==========================================
+ Hits 24159 27910 +3751
+ Misses 50279 46583 -3696
- Partials 5314 5660 +346
Continue to review full report at Codecov.
|
#7185 was merged (with no worker-api breakage) |
Should address #7146 by making it impossible to put corrupted pieces into sectors (note that after one AddPiece call fails, subsequent calls should overwrite the bad data)