-
Notifications
You must be signed in to change notification settings - Fork 2
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
Building better integration tests #65
base: main
Are you sure you want to change the base?
Conversation
4de9004
to
a877707
Compare
|
@@ -139,6 +141,29 @@ impl Storage { | |||
self.provider | |||
.get_dag_blocks_by_window(cid, offset, window_size) | |||
} | |||
|
|||
pub fn get_last_dag_cid(&self, cid: &str) -> Result<String> { | |||
let dag_cids = self.get_all_dag_cids(cid)?; |
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.
@@ -71,10 +71,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 |
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 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 ?
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.
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:
Let's assume we have a window size of 8 blocks on both sides, so we missed block 4 from that window of 1-8.
- The receiver would calculate that it has received 7 CIDs for this dag, so it would send that number in a
ResumePriorDagTransmit
message to the transmitter. - The transmitter would receive the Resume... message and determine that the window containing blocks 1...8 needs to be resent
- The whole window would be transmitted again and then the iterative windowing process would resume
Here is a slightly better way that doesn't involve reworking how we specify missing blocks:
Again assuming a window size of 8 blocks on both sides.
- The receiver would call a new function,
get_missing_blocks_and_window
, which takes a root cid and window size, and returns the last window num it was receiving blocks for, and any missing CIDs from that window (based on how large it should be), which is placed in aResumePriorDagTransmit
message and sent to the transmitter - The transmitter would send the blocks associated with the CIDs in the received
Resume...
message and set the window num in the session accordingly - After the last partly-received window has been correctly transmitted, then the iterative windowing processing resumes
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.
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.
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.
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).
a877707
to
c946193
Compare
Added tests over passes of fixed length, including turning off receiver and transmitter. Added Terminate API for shutting down listener Iterating on fixing multi-pass scenario More fixing attempts
c946193
to
25e01df
Compare
No description provided.