-
Notifications
You must be signed in to change notification settings - Fork 17
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
Refactor revalidators #308
Conversation
c364d68
to
2adcb65
Compare
078074c
to
07a03f8
Compare
Codecov Report
@@ Coverage Diff @@
## master #308 +/- ##
==========================================
- Coverage 66.06% 65.26% -0.80%
==========================================
Files 29 30 +1
Lines 3439 3501 +62
==========================================
+ Hits 2272 2285 +13
- Misses 814 850 +36
- Partials 353 366 +13
|
func (c *Channels) SetRequiresFinalization(chid datatransfer.ChannelID, RequiresFinalization bool) error { | ||
return c.send(chid, datatransfer.SetRequiresFinalization, RequiresFinalization) | ||
} |
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.
nit: RequiresFinalization => requiresFinalization
} | ||
|
||
func (m *manager) SetDataLimit(ctx context.Context, chid datatransfer.ChannelID, totalData uint64, stopAtEnd bool) { | ||
|
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.
Looks like the implementation for this function is missing
ValidatePull( | ||
isRestart bool, |
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.
IIRC we need to know at the markets layer if the pull request is for a restart. Maybe check when you implement the markets layer changes and revisit.
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.
see implementation ticket
VoucherResult | ||
// LeaveRequestPaused indicates whether the request should stay paused | ||
// even if the request was accepted | ||
LeaveRequestPaused bool |
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 suggest renaming this to Paused
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.
ultimately changed to ForcePause in subsequent PR
4006d16
to
a6e0c0e
Compare
} | ||
|
||
func (s Status) InFinalization() bool { | ||
return s == Finalizing || s == Completed || s == Completing |
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.
ResponderFinalizing
doesn't come into play here at all?
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.
ResponderFinalizing is the for the initiator, indicating the responder is finalizing. For the moment, we only care about the responder being in a finalization state, as it only affects their logic.
Probably and maybe we should add it to the list but I just want to be careful.
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 think I'll just go ahead and add it
} | ||
|
||
func (s Status) IsResponderPaused() bool { | ||
return s == ResponderPaused || s == BothPaused || s == Finalizing |
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.
ResponderFinalizing
?
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.
that's fair.
} | ||
|
||
// if we don't check data limits on this function, return | ||
if readProgress == 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.
what are the cases where this is nil
? is it just DataSent
calls? and if so, the only value in calling this function is to set the progress of the sent index, correct?
very minor nit - the naming of this function could be improved, current name doesn't imply mutation but that's one of its two core functions, updateIndexAndCheckEvents()
might be the verbose option.
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.
yea agree on all fronts. I'm inclined to wait and update when the transport refactor happens, cause I think data limit logic will move downward to the transport
} | ||
|
||
// was this the first response to our initial request | ||
if response.IsNew() { |
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 interesting, both IsNew and IsRestart jumping outside of the previously containing if
, was it wrong before not to check this in all but the IsComplete case?
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.
honestly they're just unneccesarily nested. IsNew = true & IsRestart = tree imply IsValidationResult == true, as does IsComplete = true.
So basically, the IsValidationResult handles the "!Accepted" condition and then the other things are determining behavior on an accepted response.
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.
submitting
refactor revalidation process -- removing independent revalidations, making validations results more clear
add comments to event processing and also extract functions for receiving requests to a new file
rename the IsVoucherResult to is 'IsValidationResult' also add a method for generating messages from validation results
Only dispatch Pause, Resume, SetDataLimit, and RequiresFinalization on change
Co-authored-by: dirkmc <dirkmdev@gmail.com>
Co-authored-by: dirkmc <dirkmdev@gmail.com>
Co-authored-by: dirkmc <dirkmdev@gmail.com>
Co-authored-by: Rod Vagg <rod@vagg.org>
86b6976
to
1601129
Compare
* feat(revalidators): initial refactor refactor revalidation process -- removing independent revalidations, making validations results more clear * refactor(rename): RevalidateToComplete -> RequiresFinalization * refactor(datatransfer): enhance validator comments * refactor(message): revert message response changes * refactor(impl): comment and refactor on events add comments to event processing and also extract functions for receiving requests to a new file * refactor(message): s/IsVoucherResult/IsValidationResult rename the IsVoucherResult to is 'IsValidationResult' also add a method for generating messages from validation results * feat(events): only dispatch events on change Only dispatch Pause, Resume, SetDataLimit, and RequiresFinalization on change * style(imports): fix imports * feat(events): add DataLimitExceeded event * Update channels/channel_state.go Co-authored-by: dirkmc <dirkmdev@gmail.com> * Update manager.go Co-authored-by: dirkmc <dirkmdev@gmail.com> * Update manager.go Co-authored-by: dirkmc <dirkmdev@gmail.com> * Update statuses.go Co-authored-by: Rod Vagg <rod@vagg.org> Co-authored-by: dirkmc <dirkmdev@gmail.com> Co-authored-by: Rod Vagg <rod@vagg.org>
* chore(v2): setup the v2 release given the expected breaking changes, it's time to setup the v2 release BREAKING CHANGE: v2 modules * Refactor revalidators (#308) * feat(revalidators): initial refactor refactor revalidation process -- removing independent revalidations, making validations results more clear * refactor(rename): RevalidateToComplete -> RequiresFinalization * refactor(datatransfer): enhance validator comments * refactor(message): revert message response changes * refactor(impl): comment and refactor on events add comments to event processing and also extract functions for receiving requests to a new file * refactor(message): s/IsVoucherResult/IsValidationResult rename the IsVoucherResult to is 'IsValidationResult' also add a method for generating messages from validation results * feat(events): only dispatch events on change Only dispatch Pause, Resume, SetDataLimit, and RequiresFinalization on change * style(imports): fix imports * feat(events): add DataLimitExceeded event * Update channels/channel_state.go Co-authored-by: dirkmc <dirkmdev@gmail.com> * Update manager.go Co-authored-by: dirkmc <dirkmdev@gmail.com> * Update manager.go Co-authored-by: dirkmc <dirkmdev@gmail.com> * Update statuses.go Co-authored-by: Rod Vagg <rod@vagg.org> Co-authored-by: dirkmc <dirkmdev@gmail.com> Co-authored-by: Rod Vagg <rod@vagg.org> * Refactor revalidators v2 (#322) * refactor(validators): remove revalidation move to all revalidation being asynchronous * feat(validators): implied pauses causes datalimit exceeded and requires finalization to leave request paused, regardless of where LeaveRequestPaused is set * refactor(events): reorder events reorder validation events so all get record when transfer finishes * Update impl/impl.go Co-authored-by: Rod Vagg <rod@vagg.org> Co-authored-by: Rod Vagg <rod@vagg.org> * chore(message): delete old message format code (#330) * feat(ipld): vouchers as plain ipld.Node (#325) * feat(ipld): vouchers as plain ipld.Node * feat: add ValidationResult#Equals() utility * feat(ipld): introduce TypedVoucher tuple type * chore(ipld): ipld.Node -> datamodel.Node * chore: remove RegisterVoucherResultType * fix: minor staticcheck fixes * refactor(channelstate): use cborgencompatiblenode (#336) use cborgencomptaiblenode to simply channelstate interface * feat(ipld): use bindnode/registry (#340) * chore(deps): update libp2p v0.19.4 (#341) * feat(ipld): use bindnode/registry Co-authored-by: Hannah Howard <hannah@hannahhoward.net> * refactor(v2): port graphsync w/o transport refactor Ports state machine and graphsync changes without the big transport refactor * fix(pr): respond to pr comments * chore(deps): upgrade libp2p v0.22 also upgrades graphsync and removes go-libp2p-core paths * fix(deps): use tagged go-ipld-prime Co-authored-by: dirkmc <dirkmdev@gmail.com> Co-authored-by: Rod Vagg <rod@vagg.org>
Goals
Implement #297
Implementation
The changes here are, to be blunt, not small. I recommend reviewing in chunks:
The major overall change is to the validation process. Here's our new validation interface:
What changed:
One design quirk I'm still thinking about:
So that's the main thing, but it was a big enough change I had to make a lot of code changes to do it, and that meant I had to work through a lot of code that I wrote (or others wrote) almost a year ago.... and boy it was hard not to do cleanups, especially since I had to comment to work out what it was doing again. (OY) The upside is this means the code has a lot of comments and that should make reviewing easier. Also I moved all the code related to handling request messages to it's own file, cause that has a ton of logic.
Other changes:
Core logic for data limits in OnDataQueued/OnDataReceived:
I changed the
IsVoucherResult
method toIsValidationResult
for clarity -- this removes a weird pairing ofIsVoucherResult
&EmptyVoucherResult
methods --IsVoucherResult
andEmptyVoucherResult
could be true at the same time, which was confusingAdded an imperative SendVoucherResult method. Honestly, I'd rather this not exist, but when we update the retrieval code, I want to maintain compatibility, and this will allow us to send the same information from the server we were already sending when the channel pauses
There were also some low hanging refactors that drove me crazy as I added changes -- The ChannelState implementation was holding it's own copy of the channelstate which had to be updated and copied on -- now it just embeds and internalChannelState struct.
There were two implementations of mockChannelState that I consolidated.
That's most of it. I don't think this can merge until I complete at least a draft implementation of updating the retrieval code in go-fil-markets - to verify that we actually made possible the big simplifications I'm hoping for.
Also I think this is breaking enough that it should release (along with the transport refactors) as a 2.0 package.