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

More fine grained response controls #71

Merged
merged 5 commits into from
Jul 2, 2020
Merged

Conversation

hannahhoward
Copy link
Collaborator

Goals

Allow users of module (particularly those listening through hooks) to have more fine grained control of responding to requests

Implementation

  • Add ability to pause response imperatively if you know the request ID
  • Add ability to cancel response imperatively
  • Add ability to send extensions when resuming a response (may want to include information about why it was resumed
  • Refactor out query execution code from response manager (just a simple struct extraction for now -- not putting it in a seperate module at the moment)

Copy link

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

Look good overall, two low-medium nits. Ok to merge as-is.

Comment on lines +275 to +279
// PauseResponse pauses an in progress response (may take 1 or more blocks to process)
PauseResponse(peer.ID, RequestID) error

// CancelResponse cancels an in progress response
CancelResponse(peer.ID, RequestID) error

Choose a reason for hiding this comment

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

Since this is an interface, would it perhaps make sense to proactively add ...ExtensionData here too? To reduce churn don the road...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

possibly? Gonna not do it now but may revisit

@@ -533,13 +343,47 @@ func (rm *ResponseManager) unpauseRequest(p peer.ID, requestID graphsync.Request
return nil
}

func (prm *processRequestMessage) handle(rm *ResponseManager) {
for _, request := range prm.requests {
key := responseKey{p: prm.p, requestID: request.ID()}

Choose a reason for hiding this comment

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

General observation, ignore if no time to fix wholesale:
The contraction of peerID to p not only during varnaming but also in the struct-member names is fundamentally confusing. Especially given that the rest of the struct members have full-length names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a reasonable thought. I will address seperately.

Add the ability for anyone who knows a requestID & peer to pause a response at any time
add function to directly cancel responses from requestID. also, move query execution code to
seperate struct, internal for now
Support sending extensions when resuming a request
@hannahhoward hannahhoward changed the base branch from feat/refactor-request-execution to master July 2, 2020 14:44
@hannahhoward hannahhoward merged commit 0e23085 into master Jul 2, 2020
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
@mvdan mvdan deleted the feat/response-controls branch December 15, 2021 14:15
marten-seemann pushed a commit that referenced this pull request Mar 2, 2023
* Emit events with received cids (#71)

* persist received cids on channel state.

* Send, Receive and Validate Restart requests (#75)

* Send, Receive and Validate Requests

* Initiating and Responding Tests and bug fixes (#76)

* Testing for resuming data transfer work

* Cleanup Push Restarts PR  (#79)

* cleanup of restart PR

* link the peers

* Tests for pull restarts (#84)

* tests for pull restarts

* Merge Tests cleanup work (#92)

* cleanup of restart PR

* cleanup timedout channels (#93)

* backward compatibility of restart (#96)

* backward compatibility of restart

* changes and tests

* more tests

* better error handling for restarts

* feat(message): switch to cbor map encoding (#97)

switch to cbor map encoding for the 1_1 message protocol

* feat(channels): setup datastore migrations (#99)

setup datatransfer channels so they migrate over successfully

Co-authored-by: Hannah Howard <hannah@hannahhoward.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants