You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/09/16 17:22:28 UTC
[GitHub] [cassandra-diff] yifan-c opened a new pull request #12: Allow optional query retry
yifan-c opened a new pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12
- Retry at the application level.
- Added NoRetry and ExponentialRetry strategies.
- RetryStrategy is made pluggable, as long as that the custom retry strategy extends the base and specify it in the config
- NoRetry is used if no RetryOptions is supplied in the config.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas closed pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas closed pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas commented on pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas commented on pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#issuecomment-695964465
committed as https://github.com/apache/cassandra-diff/commit/4c9bc4f4e3fd7d23b1284c89266ffbf10b8f0183
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] yifan-c commented on a change in pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#discussion_r490349333
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(Map<String, String> parameters) {
+ super(parameters);
+ long baseDelayMs = Long.parseLong(parameters.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(parameters.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+ // approximate half of totalDelayMs
+ private final long halfTotalMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ this.halfTotalMs = totalDelayMs >> 1;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according the `attempts`,i.e. [0, attempts), which is guaranteed to be greater than 0.
+ // No overflow can happen.
+ private long pausedInTotal(int attempts) {
Review comment:
Fancy! O(n) -> O(1).
Yes. It is the geometric sum. Given the ratio is 2, it is actually just `2 ^ (n + 1) - 1`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas commented on a change in pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas commented on a change in pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#discussion_r490056361
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(Map<String, String> parameters) {
+ super(parameters);
+ long baseDelayMs = Long.parseLong(parameters.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(parameters.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+ // approximate half of totalDelayMs
+ private final long halfTotalMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ this.halfTotalMs = totalDelayMs >> 1;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according the `attempts`,i.e. [0, attempts), which is guaranteed to be greater than 0.
+ // No overflow can happen.
+ private long pausedInTotal(int attempts) {
Review comment:
this looks like a geometric sum, could probably be replaced with something like:
```
private long pausedInTotal(int attempts) {
long result = -1 * baseDelayMs * (1 - (1L << attempts));
return Math.min(result, totalDelayMs);
}
```
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(Map<String, String> parameters) {
+ super(parameters);
+ long baseDelayMs = Long.parseLong(parameters.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(parameters.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+ // approximate half of totalDelayMs
+ private final long halfTotalMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ this.halfTotalMs = totalDelayMs >> 1;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according the `attempts`,i.e. [0, attempts), which is guaranteed to be greater than 0.
Review comment:
greater than or equal to 0
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(Map<String, String> parameters) {
Review comment:
should this take `JobConfiguration.RetryOptions` instead?
##########
File path: common/src/main/java/org/apache/cassandra/diff/RetryStrategyFactory.java
##########
@@ -0,0 +1,39 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.cassandra.diff.JobConfiguration.RetryOptions;
+
+public class RetryStrategyFactory {
+ public final static String IMPLEMENTATION_KEY = "impl";
+ private final static Logger logger = LoggerFactory.getLogger(RetryStrategyFactory.class);
+ private final RetryOptions retryOptions;
+
+ public RetryStrategyFactory(RetryOptions retryOptions) {
+ this.retryOptions = retryOptions;
+ }
+
+ public RetryStrategy create() {
+ if (retryOptions != null) {
+ return create(retryOptions);
+ } else {
+ logger.info("Retry is disabled.");
+ return RetryStrategy.NoRetry.INSTANCE;
+ }
+ }
+
+ public static RetryStrategy create(Map<String, String> parameters) {
Review comment:
feels slightly expensive doing this for every query - maybe we could make the strategy factory pluggable instead? Then just a factory.get() to get the actual strategy instance for every query?
##########
File path: common/src/main/java/org/apache/cassandra/diff/RetryStrategy.java
##########
@@ -0,0 +1,54 @@
+package org.apache.cassandra.diff;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.Callable;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public abstract class RetryStrategy {
+ private final static Logger logger = LoggerFactory.getLogger(RetryStrategy.class);
+
+ public RetryStrategy(Map<String, String> parameters) {
Review comment:
`JobConfiguration.RetryOptions` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas commented on a change in pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas commented on a change in pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#discussion_r490759258
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategyProvider.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategyProvider extends RetryStrategyProvider {
+ public ExponentialRetryStrategyProvider(JobConfiguration.RetryOptions retryOptions) {
+ super(retryOptions);
+ }
+
+ @Override
+ public RetryStrategy get() {
+ return new ExponentialRetryStrategy(retryOptions);
+ }
+
+ static class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(JobConfiguration.RetryOptions retryOptions) {
+ super(retryOptions);
+ long baseDelayMs = Long.parseLong(retryOptions.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(retryOptions.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
Review comment:
maybe make this constructor take the 2 longs? And move the long-parsing to the `ExponentialRetryStrategyProvider` constructor
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategyProvider.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategyProvider extends RetryStrategyProvider {
+ public ExponentialRetryStrategyProvider(JobConfiguration.RetryOptions retryOptions) {
+ super(retryOptions);
+ }
+
+ @Override
+ public RetryStrategy get() {
+ return new ExponentialRetryStrategy(retryOptions);
+ }
+
+ static class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(JobConfiguration.RetryOptions retryOptions) {
+ super(retryOptions);
+ long baseDelayMs = Long.parseLong(retryOptions.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(retryOptions.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according to the `attempts`,
+ // i.e. [0, attempts), which is guaranteed to be greater than or equal to 0.
+ // No overflow can happen.
+ private long pausedInTotal(int attempts) {
+ if (attempts >= Long.SIZE) return totalDelayMs; // take care of overflow. Such long pause time is not realistic though.
Review comment:
```if (attempts >= Long.numberOfLeadingZeros(baseDelayMs - 1)) return totalDelayMs;``` if we want to make sure no overflow can occur
##########
File path: common/src/main/java/org/apache/cassandra/diff/RetryStrategy.java
##########
@@ -0,0 +1,52 @@
+package org.apache.cassandra.diff;
+
+import java.util.concurrent.Callable;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public abstract class RetryStrategy {
+ private final static Logger logger = LoggerFactory.getLogger(RetryStrategy.class);
+
+ public RetryStrategy(JobConfiguration.RetryOptions retryOptions) {
Review comment:
can remove this constructor
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas commented on pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas commented on pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#issuecomment-695964465
committed as https://github.com/apache/cassandra-diff/commit/4c9bc4f4e3fd7d23b1284c89266ffbf10b8f0183
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas commented on a change in pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas commented on a change in pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#discussion_r490044464
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(Map<String, String> parameters) {
+ super(parameters);
+ long baseDelayMs = Long.parseLong(parameters.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(parameters.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+ // approximate half of totalDelayMs
+ private final long halfTotalMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ this.halfTotalMs = totalDelayMs >> 1;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according the `attempts`,i.e. [0, attempts), which is guaranteed to be greater than 0.
Review comment:
greater than or equal to 0
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategy.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(Map<String, String> parameters) {
+ super(parameters);
+ long baseDelayMs = Long.parseLong(parameters.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(parameters.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+ // approximate half of totalDelayMs
+ private final long halfTotalMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ this.halfTotalMs = totalDelayMs >> 1;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according the `attempts`,i.e. [0, attempts), which is guaranteed to be greater than 0.
+ // No overflow can happen.
+ private long pausedInTotal(int attempts) {
Review comment:
this looks like a geometric sum, could probably be replaced with something like:
```
private long pausedInTotalGeom(int attempts) {
long result = -1 * baseDelayMs * (1 - (1L << attempts));
return Math.min(result, totalDelayMs);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] yifan-c commented on a change in pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12#discussion_r491196386
##########
File path: common/src/main/java/org/apache/cassandra/diff/ExponentialRetryStrategyProvider.java
##########
@@ -0,0 +1,96 @@
+package org.apache.cassandra.diff;
+
+import java.util.concurrent.TimeUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.util.concurrent.Uninterruptibles;
+
+public class ExponentialRetryStrategyProvider extends RetryStrategyProvider {
+ public ExponentialRetryStrategyProvider(JobConfiguration.RetryOptions retryOptions) {
+ super(retryOptions);
+ }
+
+ @Override
+ public RetryStrategy get() {
+ return new ExponentialRetryStrategy(retryOptions);
+ }
+
+ static class ExponentialRetryStrategy extends RetryStrategy {
+ public final static String BASE_DELAY_MS_KEY = "base_delay_ms";
+ public final static String TOTAL_DELAY_MS_KEY = "total_delay_ms";
+ private final static String DEFAULT_BASE_DELAY_MS = String.valueOf(TimeUnit.SECONDS.toMillis(1));
+ private final static String DEFAULT_TOTAL_DELAY_MS = String.valueOf(TimeUnit.MINUTES.toMillis(30));
+
+ private final Exponential exponential;
+ private int attempts = 0;
+
+ public ExponentialRetryStrategy(JobConfiguration.RetryOptions retryOptions) {
+ super(retryOptions);
+ long baseDelayMs = Long.parseLong(retryOptions.getOrDefault(BASE_DELAY_MS_KEY, DEFAULT_BASE_DELAY_MS));
+ long totalDelayMs = Long.parseLong(retryOptions.getOrDefault(TOTAL_DELAY_MS_KEY, DEFAULT_TOTAL_DELAY_MS));
+ this.exponential = new Exponential(baseDelayMs, totalDelayMs);
+ }
+
+ @Override
+ protected boolean shouldRetry() {
+ long pauseTimeMs = exponential.get(attempts);
+ if (pauseTimeMs > 0) {
+ Uninterruptibles.sleepUninterruptibly(pauseTimeMs, TimeUnit.MILLISECONDS);
+ attempts += 1;
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return String.format("%s(baseDelayMs: %s, totalDelayMs: %s, currentAttempts: %s)",
+ this.getClass().getSimpleName(), exponential.baseDelayMs, exponential.totalDelayMs, attempts);
+ }
+ }
+
+ /**
+ * Calculate the pause time exponentially, according to the attempts.
+ * The total delay is capped at totalDelayMs, meaning the sum of all the previous pauses cannot exceed it.
+ */
+ static class Exponential {
+ // base delay in ms used to calculate the next pause time
+ private final long baseDelayMs;
+ // total delay in ms permitted
+ private final long totalDelayMs;
+
+ Exponential(long baseDelayMs, long totalDelayMs) {
+ Preconditions.checkArgument(baseDelayMs <= totalDelayMs, "baseDelayMs cannot be greater than totalDelayMs");
+ this.baseDelayMs = baseDelayMs;
+ this.totalDelayMs = totalDelayMs;
+ }
+
+ /**
+ * Calculate the pause time based on attempts.
+ * It is guaranteed that the all the pauses do not exceed totalDelayMs.
+ * @param attempts, number of attempts, starts with 0.
+ * @return the next pasuse time in milliseconds, or negtive if no longer allowed.
+ */
+ long get(int attempts) {
+ long nextMaybe = baseDelayMs * (1L << attempts); // Do not care about overflow. pausedInTotal() corrects the value
+ if (attempts == 0) { // first retry
+ return nextMaybe;
+ } else {
+ long pausedInTotal = pausedInTotal(attempts);
+ if (pausedInTotal < totalDelayMs) {
+ return Math.min(totalDelayMs - pausedInTotal, nextMaybe); // adjust the next pause time if possible
+ }
+ return -1; // the previous retries have exhausted the permits
+ }
+ }
+
+ // Returns the total pause time according to the `attempts`,
+ // i.e. [0, attempts), which is guaranteed to be greater than or equal to 0.
+ // No overflow can happen.
+ private long pausedInTotal(int attempts) {
+ if (attempts >= Long.SIZE) return totalDelayMs; // take care of overflow. Such long pause time is not realistic though.
Review comment:
mostly agree. It should have taken `baseDelayMs` into consideration.
However, I do not think the subtraction is needed. The first attempt to start to return the `total` directly is the one that could make the value wraps around, from a positive to a negative.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-diff] krummas closed pull request #12: CASSANDRA-16125: Allow optional query retry
Posted by GitBox <gi...@apache.org>.
krummas closed pull request #12:
URL: https://github.com/apache/cassandra-diff/pull/12
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org