[server] Add retriable authentication exception. #845
Conversation
| @@ -319,7 +319,7 @@ private void handleAuthenticateRequest( | |||
| if (!authenticator.isCompleted()) { | |||
| byte[] token = authenticateRequest.getToken(); | |||
| byte[] challenge = authenticator.evaluateResponse(token); | |||
| if (!authenticator.isCompleted() && challenge != null) { | |||
There was a problem hiding this comment.
Is the removal of !authenticator.isCompleted() necessary? I though the authenticator.evaluateResponse may change status of authenticator into complete and don't need to send challenge.
| } | ||
|
|
||
| private void handleAuthenticateResponse(ApiMessage response, Throwable cause) { | ||
| if (cause != null) { |
There was a problem hiding this comment.
Can we still authenticate in the ServerConnection instead of a separate channel handler? It increases the complexity that the authenticator and switchState out of the lock and may cause problems.
I though we only need to re-auth here when the cause is retrable auth exception. In the future, we should also add re-auth logic here for expired authentications. We can also maintain backoff in ServerConnection as it is per-connection.
| new AuthenticateRequest() | ||
| .setToken(token) | ||
| .setProtocol(authenticator.protocol()); | ||
| pendingRequest = request; |
There was a problem hiding this comment.
Is it necessary to remember the auth request? Can we just re-send the auth request from byte[0]? We should try to avoid maintaining states.
| private void handleAuthenticateResponse(ApiMessage response, Throwable cause) { | ||
| if (cause != null) { | ||
| if (cause instanceof RetriableAuthenticationException | ||
| && retryAuthNum < MAX_RETRY_AUTH_NUM) { |
There was a problem hiding this comment.
We don't need to set max retry num and allow the request timeout by netty client. Sender also doesn't have max retry on RetriableException.
| } else { | ||
| callback.onAuthenticateFailure( | ||
| new IllegalStateException( | ||
| new IllegalStateException( |
There was a problem hiding this comment.
Why nested IllegalStateException?
| private static final Logger LOG = LoggerFactory.getLogger(NettyAuthenticationHandler.class); | ||
| private static final Random RANDOM = new Random(); | ||
| private static final long BASE_BACKOFF_MS = 500L; | ||
| private static final long MAX_BACKOFF_MS = 60000L; |
There was a problem hiding this comment.
BASE_BACKOFF_MS -> 100L (better initial latency) and MAX_BACKOFF_MS -> 5s (not idle too long)
| private long calculateBackOff() { | ||
| // Exponential backoff with jitter | ||
| long backOff = BASE_BACKOFF_MS * (1L << retryAuthNum); | ||
| backOff = Math.min(backOff, MAX_BACKOFF_MS); // Ensure it does not exceed maxBackOffMs | ||
| long jitter = (long) (backOff * JITTER_FACTOR * RANDOM.nextDouble()); | ||
| return backOff + jitter; | ||
| } |
There was a problem hiding this comment.
Introduce Kafka ExponentialBackoff and ExponentialBackoffTest for the verified backoff util. The jitter should consider -20% ~ + 20%.
|
Impressive project structure! The architecture really demonstrates solid design principles. |
|
The performance optimizations here are quite clever. Nice engineering work! |
|
This is a well-structured codebase. I appreciate the clean separation of concerns. |
|
The performance optimizations here are quite clever. Nice engineering work! |
|
This is a well-structured codebase. I appreciate the clean separation of concerns. |
Purpose
Linked issue: close #844
Brief change log
Tests
com.alibaba.fluss.rpc.netty.authenticate.AuthenticationTest#testRetirableAuthenticateException
API and Format
Documentation