Copilot commented on code in PR #34290:
URL: https://github.com/apache/superset/pull/34290#discussion_r2226845054
##########
superset-frontend/src/setup/setupClient.ts:
##########
@@ -31,12 +31,44 @@ function getDefaultConfiguration(): ClientConfig {
bootstrapData.common.conf.JWT_ACCESS_CSRF_COOKIE_NAME;
const cookieCSRFToken = parseCookie()[jwtAccessCsrfCookieName] || '';
+ // Configure retry behavior from backend settings
+ const retryConfig = bootstrapData.common.conf;
+
+ // Create exponential backoff delay function with jitter
+ const createRetryDelayFunction = () => {
+ const baseDelay = retryConfig.SUPERSET_CLIENT_RETRY_DELAY || 1000;
+ const multiplier =
+ retryConfig.SUPERSET_CLIENT_RETRY_BACKOFF_MULTIPLIER || 2;
+ const maxDelay = retryConfig.SUPERSET_CLIENT_RETRY_MAX_DELAY || 10000;
+ const jitterMax = retryConfig.SUPERSET_CLIENT_RETRY_JITTER_MAX || 1000;
+
+ return (attempt: number) => {
+ // Calculate exponential backoff: baseDelay * (multiplier ^ attempt)
+ const exponentialDelay = baseDelay * multiplier ** attempt;
+
+ // Apply max delay cap
+ const cappedDelay = Math.min(exponentialDelay, maxDelay);
+
+ // Add random jitter to prevent thundering herd
+ const jitter = Math.random() * jitterMax;
Review Comment:
[nitpick] Using Math.random() for jitter provides uniform distribution, but
exponential jitter (random between 0 and calculated delay) is often preferred
for retry mechanisms. Consider implementing exponential jitter: `Math.random()
* cappedDelay` instead of fixed jitter.
```suggestion
const jitter = Math.random() * cappedDelay;
```
##########
superset-frontend/src/setup/setupClient.ts:
##########
@@ -31,12 +31,44 @@ function getDefaultConfiguration(): ClientConfig {
bootstrapData.common.conf.JWT_ACCESS_CSRF_COOKIE_NAME;
const cookieCSRFToken = parseCookie()[jwtAccessCsrfCookieName] || '';
+ // Configure retry behavior from backend settings
+ const retryConfig = bootstrapData.common.conf;
+
+ // Create exponential backoff delay function with jitter
+ const createRetryDelayFunction = () => {
+ const baseDelay = retryConfig.SUPERSET_CLIENT_RETRY_DELAY || 1000;
+ const multiplier =
+ retryConfig.SUPERSET_CLIENT_RETRY_BACKOFF_MULTIPLIER || 2;
+ const maxDelay = retryConfig.SUPERSET_CLIENT_RETRY_MAX_DELAY || 10000;
+ const jitterMax = retryConfig.SUPERSET_CLIENT_RETRY_JITTER_MAX || 1000;
+
+ return (attempt: number) => {
+ // Calculate exponential backoff: baseDelay * (multiplier ^ attempt)
+ const exponentialDelay = baseDelay * multiplier ** attempt;
Review Comment:
The exponential calculation uses the exponentiation operator which could
result in very large delays for higher attempt numbers. Consider adding bounds
checking or using Math.pow() with explicit overflow handling to prevent
unexpected behavior.
```suggestion
// Calculate exponential backoff: baseDelay * Math.pow(multiplier,
attempt)
const safeAttempt = Math.min(attempt, 10); // Limit attempt to prevent
overflow
const exponentialDelay = baseDelay * Math.pow(multiplier, safeAttempt);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
