Skip to content

Commit 8cd86f3

Browse files
Lorak-mmkLorak-mmk
authored
Merge pull request #1083 from muzarski/internal-error-matches-no-wildcard
error: use explicit matches for errors when the decision is crucial for correctness/performance
2 parents fdb9691 + e9f0b15 commit 8cd86f3

File tree

4 files changed

+249
-134
lines changed

4 files changed

+249
-134
lines changed

‎scylla/src/policies/load_balancing/default.rs‎

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2854,27 +2854,65 @@ mod latency_awareness {
28542854
}
28552855

28562856
pub(crate) fn reliable_latency_measure(error: &RequestAttemptError) -> bool {
2857+
// Do not remove this lint!
2858+
// It's there for a reason - we don't want new variants
2859+
// automatically fall under `_` pattern when they are introduced.
2860+
#[deny(clippy::wildcard_enum_match_arm)]
28572861
match error {
2858-
// "fast" errors, i.e. ones that are returned quickly after the query begins
2862+
// "fast" errors, i.e. ones that appeared on driver side, before sending the request
28592863
RequestAttemptError::CqlRequestSerialization(_)
28602864
| RequestAttemptError::BrokenConnectionError(_)
28612865
| RequestAttemptError::UnableToAllocStreamId
2862-
| RequestAttemptError::DbError(DbError::IsBootstrapping, _)
2863-
| RequestAttemptError::DbError(DbError::Unavailable { .. }, _)
2864-
| RequestAttemptError::DbError(DbError::Unprepared { .. }, _)
2865-
| RequestAttemptError::DbError(DbError::Overloaded, _)
2866-
| RequestAttemptError::DbError(DbError::RateLimitReached { .. }, _)
28672866
| RequestAttemptError::SerializationError(_) => false,
28682867

28692868
// "slow" errors, i.e. ones that are returned after considerable time of query being run
2870-
RequestAttemptError::DbError(_, _)
2871-
| RequestAttemptError::CqlResultParseError(_)
2869+
RequestAttemptError::CqlResultParseError(_)
28722870
| RequestAttemptError::CqlErrorParseError(_)
28732871
| RequestAttemptError::BodyExtensionsParseError(_)
28742872
| RequestAttemptError::RepreparedIdChanged { .. }
28752873
| RequestAttemptError::RepreparedIdMissingInBatch
28762874
| RequestAttemptError::UnexpectedResponse(_)
28772875
| RequestAttemptError::NonfinishedPagingState => true,
2876+
2877+
// handle db errors
2878+
RequestAttemptError::DbError(db_error, _) => {
2879+
// Do not remove this lint!
2880+
// It's there for a reason - we don't want new variants
2881+
// automatically fall under `_` pattern when they are introduced.
2882+
#[deny(clippy::wildcard_enum_match_arm)]
2883+
match db_error {
2884+
// An errors that appeared on server side, but did not require selected node
2885+
// to contact other nodes (i.e. fast server side errors).
2886+
DbError::IsBootstrapping
2887+
| DbError::Unavailable { .. }
2888+
| DbError::Unprepared { .. }
2889+
| DbError::Overloaded
2890+
| DbError::RateLimitReached { .. } => false,
2891+
2892+
// Rest of server side errors that take considerable amount of time to be returned
2893+
DbError::SyntaxError
2894+
| DbError::Invalid
2895+
| DbError::AlreadyExists { .. }
2896+
| DbError::FunctionFailure { .. }
2897+
| DbError::AuthenticationError
2898+
| DbError::TruncateError
2899+
| DbError::ReadTimeout { .. }
2900+
| DbError::WriteTimeout { .. }
2901+
| DbError::ReadFailure { .. }
2902+
| DbError::WriteFailure { .. }
2903+
| DbError::ServerError
2904+
| DbError::ProtocolError
2905+
| DbError::Unauthorized
2906+
| DbError::ConfigError
2907+
| DbError::Other(_) => true,
2908+
2909+
// Driver may be used with newer version of scylla-cql, which may define additional error variants.
2910+
// It is better to not include unknown errors in latency measure.
2911+
// `wildcard_enum_match_arm will` will still trigger if we add new variants to DbError,
2912+
// so there is no risk of forgetting to update this match.
2913+
_ => false,
2914+
}
2915+
}
28782916
}
28792917
}
28802918
}

‎scylla/src/policies/retry/default.rs‎

Lines changed: 95 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -59,77 +59,113 @@ impl RetrySession for DefaultRetrySession {
5959
if request_info.consistency.is_serial() {
6060
return RetryDecision::DontRetry;
6161
};
62+
// Do not remove this lint!
63+
// It's there for a reason - we don't want new variants
64+
// automatically fall under `_` pattern when they are introduced.
65+
#[deny(clippy::wildcard_enum_match_arm)]
6266
match request_info.error {
63-
// Basic errors - there are some problems on this node
64-
// Retry on a different one if possible
65-
RequestAttemptError::BrokenConnectionError(_)
66-
| RequestAttemptError::DbError(DbError::Overloaded, _)
67-
| RequestAttemptError::DbError(DbError::ServerError, _)
68-
| RequestAttemptError::DbError(DbError::TruncateError, _) => {
67+
// With connection broken, we don't know if request was executed.
68+
RequestAttemptError::BrokenConnectionError(_) => {
6969
if request_info.is_idempotent {
7070
RetryDecision::RetryNextTarget(None)
7171
} else {
7272
RetryDecision::DontRetry
7373
}
7474
}
75-
// Unavailable - the current node believes that not enough nodes
76-
// are alive to satisfy specified consistency requirements.
77-
// Maybe this node has network problems - try a different one.
78-
// Perform at most one retry - it's unlikely that two nodes
79-
// have network problems at the same time
80-
RequestAttemptError::DbError(DbError::Unavailable { .. }, _) => {
81-
if !self.was_unavailable_retry {
82-
self.was_unavailable_retry = true;
83-
RetryDecision::RetryNextTarget(None)
84-
} else {
85-
RetryDecision::DontRetry
86-
}
87-
}
88-
// ReadTimeout - coordinator didn't receive enough replies in time.
89-
// Retry at most once and only if there were actually enough replies
90-
// to satisfy consistency but they were all just checksums (data_present == false).
91-
// This happens when the coordinator picked replicas that were overloaded/dying.
92-
// Retried request should have some useful response because the node will detect
93-
// that these replicas are dead.
94-
RequestAttemptError::DbError(
95-
DbError::ReadTimeout {
96-
received,
97-
required,
98-
data_present,
99-
..
100-
},
101-
_,
102-
) => {
103-
if !self.was_read_timeout_retry && received >= required && !*data_present {
104-
self.was_read_timeout_retry = true;
105-
RetryDecision::RetrySameTarget(None)
106-
} else {
107-
RetryDecision::DontRetry
108-
}
109-
}
110-
// Write timeout - coordinator didn't receive enough replies in time.
111-
// Retry at most once and only for BatchLog write.
112-
// Coordinator probably didn't detect the nodes as dead.
113-
// By the time we retry they should be detected as dead.
114-
RequestAttemptError::DbError(DbError::WriteTimeout { write_type, .. }, _) => {
115-
if !self.was_write_timeout_retry
116-
&& request_info.is_idempotent
117-
&& *write_type == WriteType::BatchLog
118-
{
119-
self.was_write_timeout_retry = true;
120-
RetryDecision::RetrySameTarget(None)
121-
} else {
122-
RetryDecision::DontRetry
75+
// DbErrors
76+
RequestAttemptError::DbError(db_error, _) => {
77+
// Do not remove this lint!
78+
// It's there for a reason - we don't want new variants
79+
// automatically fall under `_` pattern when they are introduced.
80+
#[deny(clippy::wildcard_enum_match_arm)]
81+
match db_error {
82+
// Basic errors - there are some problems on this node
83+
// Retry on a different one if possible
84+
DbError::Overloaded | DbError::ServerError | DbError::TruncateError => {
85+
if request_info.is_idempotent {
86+
RetryDecision::RetryNextTarget(None)
87+
} else {
88+
RetryDecision::DontRetry
89+
}
90+
}
91+
// Unavailable - the current node believes that not enough nodes
92+
// are alive to satisfy specified consistency requirements.
93+
// Maybe this node has network problems - try a different one.
94+
// Perform at most one retry - it's unlikely that two nodes
95+
// have network problems at the same time
96+
DbError::Unavailable { .. } => {
97+
if !self.was_unavailable_retry {
98+
self.was_unavailable_retry = true;
99+
RetryDecision::RetryNextTarget(None)
100+
} else {
101+
RetryDecision::DontRetry
102+
}
103+
}
104+
// ReadTimeout - coordinator didn't receive enough replies in time.
105+
// Retry at most once and only if there were actually enough replies
106+
// to satisfy consistency but they were all just checksums (data_present == false).
107+
// This happens when the coordinator picked replicas that were overloaded/dying.
108+
// Retried request should have some useful response because the node will detect
109+
// that these replicas are dead.
110+
DbError::ReadTimeout {
111+
received,
112+
required,
113+
data_present,
114+
..
115+
} => {
116+
if !self.was_read_timeout_retry && received >= required && !*data_present {
117+
self.was_read_timeout_retry = true;
118+
RetryDecision::RetrySameTarget(None)
119+
} else {
120+
RetryDecision::DontRetry
121+
}
122+
}
123+
// Write timeout - coordinator didn't receive enough replies in time.
124+
// Retry at most once and only for BatchLog write.
125+
// Coordinator probably didn't detect the nodes as dead.
126+
// By the time we retry they should be detected as dead.
127+
DbError::WriteTimeout { write_type, .. } => {
128+
if !self.was_write_timeout_retry
129+
&& request_info.is_idempotent
130+
&& *write_type == WriteType::BatchLog
131+
{
132+
self.was_write_timeout_retry = true;
133+
RetryDecision::RetrySameTarget(None)
134+
} else {
135+
RetryDecision::DontRetry
136+
}
137+
}
138+
// The node is still bootstrapping it can't execute the request, we should try another one
139+
DbError::IsBootstrapping => RetryDecision::RetryNextTarget(None),
140+
// In all other cases propagate the error to the user
141+
DbError::SyntaxError
142+
| DbError::Invalid
143+
| DbError::AlreadyExists { .. }
144+
| DbError::FunctionFailure { .. }
145+
| DbError::AuthenticationError
146+
| DbError::Unauthorized
147+
| DbError::ConfigError
148+
| DbError::ReadFailure { .. }
149+
| DbError::WriteFailure { .. }
150+
| DbError::Unprepared { .. }
151+
| DbError::ProtocolError
152+
| DbError::RateLimitReached { .. }
153+
| DbError::Other(_)
154+
| _ => RetryDecision::DontRetry,
123155
}
124156
}
125-
// The node is still bootstrapping it can't execute the request, we should try another one
126-
RequestAttemptError::DbError(DbError::IsBootstrapping, _) => {
127-
RetryDecision::RetryNextTarget(None)
128-
}
129157
// Connection to the contacted node is overloaded, try another one
130158
RequestAttemptError::UnableToAllocStreamId => RetryDecision::RetryNextTarget(None),
131159
// In all other cases propagate the error to the user
132-
_ => RetryDecision::DontRetry,
160+
RequestAttemptError::BodyExtensionsParseError(_)
161+
| RequestAttemptError::CqlErrorParseError(_)
162+
| RequestAttemptError::CqlRequestSerialization(_)
163+
| RequestAttemptError::CqlResultParseError(_)
164+
| RequestAttemptError::NonfinishedPagingState
165+
| RequestAttemptError::RepreparedIdChanged { .. }
166+
| RequestAttemptError::RepreparedIdMissingInBatch
167+
| RequestAttemptError::SerializationError(_)
168+
| RequestAttemptError::UnexpectedResponse(_) => RetryDecision::DontRetry,
133169
}
134170
}
135171

0 commit comments

Comments
 (0)