-
Notifications
You must be signed in to change notification settings - Fork 2
Building better integration tests #65
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,10 +82,22 @@ pub enum ApplicationAPI { | |
| }, | ||
| // Resumes the transmission of all dags which may be paused | ||
| ResumeTransmitAllDags, | ||
| // Resumes the transmission of a dag from a prior session, given the last received CID | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, you don't provide the last received CID. Since UDP can go out-of-order and drop middle parts, I wonder if there might be another model that could be followed. Like if you got block 1,2,3,5,6,7 & 8 but missed 4, do you really want to ask for everything starting with 4 to be retransmitted? Couldn't you just as easily start a request for a new DAG rooted at CID of block # 4 ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I'm not sure we could start a transmission for a DAG rooted at the CID of block 4, because it wouldn't have the links to resolve the rest of the DAG like the root CID would. But I do think we could use some windowing information to make the first resume request a bit better. Here is the current behavior for the scenario you laid out:
Here is a slightly better way that doesn't involve reworking how we specify missing blocks:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify: I'm not suggesting a change here and now. Just thinking about these things. As I wrap my head around the project I can't help but noodle a bit.
It would only have its own block and children (grand children of the original root, perhaps) if it has any. Every node in a DAG is a root of a smaller DAG, because 🌴 😄 What I was really getting at is one could have a request that said, "Here's some CIDs I want" and pass in the missing CIDs, and that's logically the same as calling TransmitDag (with your own address as the target) repeatedly - once for every link in the DAG you don't have a block for. But yeah, if you have a window as a parameter in the request message, then yes you can do the exact equivalent thing. If you can detect a missing block that implies you have a block with a link to it, so you could just as easily 'resume' that block with a window that only covers links you know you don't have. I don't know if you need the transmitter to initiate the resume stuff, though? myceli could see what blocks it's missing at startup (I imagine when the sat gets powered on it runs main(), yes?) and just start requesting them. Or if that's too much (might accidentally un-GC you), one could have a database record of roots of DAGs that were originally sent, and at startup start requesting links missing only from them (or if the DAG is complete remove it from the table). |
||
| // for determining where to restart the transmission | ||
| ResumePriorDagTransmit { | ||
| cid: String, | ||
| num_received_cids: u32, | ||
| retries: u8, | ||
| }, | ||
| /// Listens on address for data and writes out files received | ||
| Receive { | ||
| listen_addr: String, | ||
| }, | ||
| /// Commands a node to request another node at target_addr to resume dag transfer | ||
| RequestResumeDagTransfer { | ||
| cid: String, | ||
| target_addr: String, | ||
| }, | ||
| /// Request Available Blocks | ||
| RequestAvailableBlocks, | ||
| /// Advertise all available blocks by CID | ||
|
|
@@ -125,6 +137,8 @@ pub enum ApplicationAPI { | |
| AvailableDags { | ||
| dags: Vec<DagInfo>, | ||
| }, | ||
| /// Asks IPFS instance to terminate | ||
| Terminate, | ||
| // TODO: Implement later | ||
| // Information about the next pass used for calculating | ||
| // data transfer parameters | ||
|
|
||
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 feel like there should be a TODO to have the get_all* functions return an iterator rather than allocate. This is a good example of why.
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 this could easily have been done inside of sqlite instead of returning all of the CIDs...but this function doesn't end up really getting used, so it can probably get removed.
But yes I agree that a good refactor would be returning iterator from those
get_all*functions.