Better to do tests in Callback. In the current implementation, we can't know whether split fails or it just is waitting for executing.

done

Better to extract and test the error code.

done

Better to rename leader, follower_1 to peer_1, peer_2, ...

done

Why not call unwrap to simplify the code?

done

Checking an atomic for every raft message and proposal may bring extra cost.

Maybe we can reduce the cost by only check the atomic once for each batch of peer, more specifically setting a flag like poller_ctx.is_disk_full at PollHandler::begin and then checking the poller_ctx.is_disk_full for each raft message and proposal instead.

This may bring little delay to detect disk full but I think the delay will be much shorter than the disk stats monitor interval (1s).

Also, I found this PR didn't cover snapshot generating and receiving, those could also consume disk space.

Yes, currently, these are also not limited in this PR: snapshot, running log, rocksdb compaction and wal, etc.
Maybe a good strategy is to solve the main problem first, and then observe the online status.

Checking an atomic for every raft message and proposal may bring extra cost.

Maybe we can reduce the cost by only check the atomic once for each batch of peer, more specifically setting a flag like poller_ctx.is_disk_full at PollHandler::begin and then checking the poller_ctx.is_disk_full for each raft message and proposal instead.

This may bring little delay to detect disk full but I think the delay will be much shorter than the disk stats monitor interval (1s).

The impact of access to atomic variables on performance should not be so big, we can add a performance test observation.

Not just performance but also resource cost (like CPU), the impact may not big enough to be noticeable but I think it is an unnecessary cost though.

Looks like disk_full_precheck is always use as disk_full_precheck || is_disk_full, how about combining these two into one like:

pub fn is_disk_full() -> bool {
    fail_point!("disk_full_peer_1", _store_id == 1, |_| true);
    fail_point!("disk_full_peer_2", _store_id == 2, |_| true);
    DISK_FULL.load(Ordering::Acquire)
}

Maybe not. Because disk_full_precheck only used to ut, if we merge both, then the disk stats worker will clean it up periodically.

And there will be a more obscure logic, that is, in other places where precheck is not needed, you have to pass a store_id to this merged function.