-
Notifications
You must be signed in to change notification settings - Fork 20
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 support for COPY IN protocol #72
Conversation
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.
Thanks for opening a PR! Left a few comments. Would like to explore if we could expose the copy implementation as a io
interface.
0af1145
to
3325a8a
Compare
and add a bit more debugging
…ped through one command at a time
Much thanks @flimzy! I have implemented the last changes as discussed in the comments. This PR is ready to be merged! |
Thank you, @jeroenrinzema , for picking up the ball where I dropped it. And thanks for the great package! |
This PR is still a bit rough. It does not have tests for corner cases (most important: when client sends CopyFail), and I'm not entirely sure that the structure is as simple as it can be.
But I'd love to get some feedback on the approach.
Two main API changes:
CopyIn
added to theDataWriter
type.Execute
method in thePortalCache
interface needs an additional argument. I've implemented this in a backward-compatible way, though.Still to do:
CopyFail
?CopyFail
from clientNote that this does not implement
CopyOutResponse
orCopyBothResponse
. I don't (yet) need either of these. But with the groundwork in this PR, they should both be easier to accomplish if/when needed.