opt: optimize cluster identification #3032
Conversation
tediou5
requested review from
nazar-pc,
shamil-gadelshin and
rg3l3dr
as code owners
|
This looks like a known issue in |
| @@ -32,8 +34,6 @@ use tracing::{error, info, trace, warn}; | |||
| type AddRemoveFuture<'a> = | |||
| Pin<Box<dyn Future<Output = Option<(FarmIndex, oneshot::Receiver<()>, ClusterFarm)>> + 'a>>; | |||
|
|
|||
| pub(super) type FarmIndex = u16; | |||
There was a problem hiding this comment.
This was meant to be a generic, cluster doesn't need to know what the type is, we could have used u8, but decided to allow for more than 256 farms in cluster mode. Why was this necessary?
There was a problem hiding this comment.
just move to subspace_farmer::cluster::farmer, because I want to use it in ClusterFarmerIdentifyBroadcast
There was a problem hiding this comment.
There should be no need to send it in there, it seems to be an artifact of ID derivation mechanism
|
|
||
| /// Derive sub IDs | ||
| #[inline] | ||
| pub fn derive_sub_ids(&self, n: usize) -> Vec<Self> { |
There was a problem hiding this comment.
This makes no sense to me, can you explain what this is supposed to mean?
There was a problem hiding this comment.
Using a child id derived from a farmer or cache avoids the need to transfer farm ids, which would make it redundant to randomize new ids for each farm when the farmer or cache is already randomly generating ids.
There was a problem hiding this comment.
Still makes no sense to me, sorry.
This is farm ID, just one of the many farms the farmer is managing. Other farms have different IDs. I don't understand what this derivation has to do with any of that.
There was a problem hiding this comment.
I can remove him if you don't think it's necessary, it doesn't affect the overall logic here
There was a problem hiding this comment.
I'm trying to understand why it was added in the first place and so far it doesn't really make sense to me
There was a problem hiding this comment.
This does make it confusing, maybe I should change the type to FarmerId.
There was a problem hiding this comment.
It makes no sense to me either way. You need to transfer the same information that was transferred before these changes anyway.
There was a problem hiding this comment.
Either way we need to know what farms are inside the farmer, recorded or derived, so you prefer to record the farms_id inside the farmer in the controller.
There was a problem hiding this comment.
There are already farm_ids of individual farms in the controller. What needs to be is to group them by farmer ID somehow and only transfer details once during initialization.
There was a problem hiding this comment.
I know what you mean. I'll try it later.
Hmm, it failed again: Not sure if this PR, other recent changes, or infrastructure changes are triggering it. |
I will check it later |
|
Closing due to lack of progress and numerous merge conflicts. Feel free to reopen later. We also have improved infrastructure for stream requests now resulting in less boilerplate. |
|
This didn’t seem to actually get closed, I’ll do it now. |
close #2900
For the cache, the modification is quite simple — just generate a random Cache ID each time and derive sub-IDs from it.
However, it's a bit different for the farmer. Currently, the logic for deriving the farmer's fingerprint is almost the same as that for the farm's fingerprint, but the information included is insufficient to fully describe whether the farm's state has changed between two restarts.
As a result, the current implementation does not support fast restarts. However, we have still achieved our initial goal: reducing identification messages to one per farmer and retrieving farm details in a steady stream.
Therefore, I will submit this PR first and implement the fast restart feature in a subsequent PR.
Code contributor checklist: