Tweak a retry number for piece retrieval operations on DSN L2. #1910
Conversation
- Move retry number const for piece retrieval to subspace-networking crate.
| pub use utils::multihash::Multihash; | ||
| pub use utils::prometheus::start_prometheus_metrics_server; | ||
| pub use utils::unique_record_binary_heap::{KeyWrapper, UniqueRecordBinaryHeap}; | ||
|
|
||
| /// Get piece retry attempts number. | ||
| pub const PIECE_GETTER_RETRY_NUMBER: NonZeroU16 = NonZeroU16::new(3).expect("Not zero; qed"); |
There was a problem hiding this comment.
If you increase number of retries beyond one you can immediately expect me to ask for rationale and explanation why it is necessary and which issues does it fix.
Also I really don't like non-fundamental values as constants, especially public ones.
There was a problem hiding this comment.
What do you mean by "non-fundamental values"?
If you increase number of retries beyond one you can immediately expect me to ask for rationale and explanation why it is necessary and which issues does it fix.
We discussed that yesterday, here are key takeaways: dsn cache scan without retries has 1-3% "not found" results on my machine. Setting this value to 3 makes scans 100% successful. I set this value to increase the probability of maintaining a healthy cache across the network. I left piece retrieval on block importing and piece reconstruction with their zero retries because they just retry with different piece indexes. In other cases, it makes sense to use the recommended value. Also, if this value changes - we'll change it in a single place.
There was a problem hiding this comment.
What do you mean by "non-fundamental values"?
We have fundamental protocol constants like piece size for instance.
Retries though are not fundamental number and should be a part of dynamic configuration. You can set default to some number, but it should still be a configuration parameter, not a constant exposed on subspace_networking. As such I don't think it should be a single number in a single place.
So does this mean we're making up to 4 attempts to download a piece (initial + 3 retries)?
There was a problem hiding this comment.
BTW your last comment is exactly the kind of details I expect to be written in PR description from the beginning. One generic line with zero specifics doesn't cut it.
There was a problem hiding this comment.
Retries though are not fundamental number and should be a part of dynamic configuration. You can set default to some number, but it should still be a configuration parameter, not a constant exposed on subspace_networking. As such I don't think it should be a single number in a single place.
Ok. I will change it back to crate's local constants.
So does this mean we're making up to 4 attempts to download a piece (initial + 3 retries)?
Yes, in rare cases. In practice, we need at most 2 attempts.
We didn't change the protocol enough to break these requests. I would guess that is because of either "behind the NAT yet present in Kademlia peers" or a natural churn. |
Also removal of cuckoo filters broke compatibility with all older clients because cuckoo library panicked internally with zero sized filters. So I have to disagree, we did broke it. |
Hmm, both issues could contribute to the failed requests indeed. |
Tweak a retry number for piece retrieval operations on DSN L2. (#1910)
This PR tweaks a retry number for piece retrieval operations on L2 cache population.
Code contributor checklist: