Xline icon indicating copy to clipboard operation
Xline copied to clipboard

improve the maintainability of `is_conflict` method

Open Phoenix500526 opened this issue 1 year ago • 0 comments

Currently, the is_conflict method in xine/src/server/command.rs is challenging to maintain, primarily due to the following reasons:

impl ConflictCheck for Command {
    #[inline]
    fn is_conflict(&self, other: &Self) -> bool {
        if self.id == other.id {
            return true;
        }
        let this_req = &self.request.request;
        let other_req = &other.request.request;
        // auth read request will not conflict with any request except the auth write request
        if (this_req.is_auth_read_request() && other_req.is_auth_read_request())
            || (this_req.is_kv_request() && other_req.is_auth_read_request())
            || (this_req.is_auth_read_request() && other_req.is_kv_request())
        {
            return false;
        }
        // any two requests that don't meet the above conditions will conflict with each other
        // because the auth write request will make all previous token invalid
        if (this_req.is_auth_request()) || (other_req.is_auth_request()) {
            return true;
        }

        // Lease leases request is conflict with Lease grant and revoke requests
        if (this_req.is_lease_read_request() && other_req.is_lease_write_request())
            || (this_req.is_lease_write_request() && other_req.is_lease_read_request())
        {
            return true;
        }

        if this_req.is_compaction_request() && other_req.is_compaction_request() {
            return true;
        }

        if (this_req.is_txn_request() && other_req.is_compaction_request())
            || (this_req.is_compaction_request() && other_req.is_txn_request())
        {
            match (this_req, other_req) {
                (
                    &RequestWrapper::CompactionRequest(ref com_req),
                    &RequestWrapper::TxnRequest(ref txn_req),
                )
                | (
                    &RequestWrapper::TxnRequest(ref txn_req),
                    &RequestWrapper::CompactionRequest(ref com_req),
                ) => {
                    let target_revision = com_req.revision;
                    return txn_req.is_conflict_with_rev(target_revision)
                }
                _ => unreachable!("The request must be either a transaction or a compaction request! \nthis_req = {this_req:?} \nother_req = {other_req:?}")
            }
        }

        let this_lease_ids = get_lease_ids(this_req);
        let other_lease_ids = get_lease_ids(other_req);
        let lease_conflict = !this_lease_ids.is_disjoint(&other_lease_ids);
        let key_conflict = self
            .keys()
            .iter()
            .cartesian_product(other.keys().iter())
            .any(|(k1, k2)| k1.is_conflicted(k2));
        lease_conflict || key_conflict
    }
}
  1. Some obviously conflicting requests require multiple conditional checks to determine the result. For example, two CompactionRequest are clearly conflicting, but they must pass the first three if statements before the control flow hit the right condition.
  2. With the increase in requests, the possibilities of matches grow rapidly. For instance, with 10 different requests, there are 100 possible combinations of (this_req, other_req). Adding one more request increases the total combinations to (10 + 1)^2 = 121. Therefore, we need an intuitive declarative approach to describe whether different combinations result in conflicts. One possible solution is to use macros for abstraction.

Phoenix500526 avatar Jul 13 '23 07:07 Phoenix500526