abbccdda commented on a change in pull request #8846: URL: https://github.com/apache/kafka/pull/8846#discussion_r439549625
########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... + * The formula is Term(n) = random(1 - jitter, 1 + jitter) * scaleFactor * (ratio) ^ n + * If scaleFactor is greater or equal than termMax, a constant term of will be provided + * This class is thread-safe + */ +public class GeometricProgression { + private final int ratio; + private final double expMax; + private final long scaleFactor; + private final double jitter; + + public GeometricProgression(long scaleFactor, int ratio, long termMax, double jitter) { + this.scaleFactor = scaleFactor; Review comment: nit: align the order of initialization with parameter list. ########## File path: clients/src/main/java/org/apache/kafka/clients/admin/AdminClientConfig.java ########## @@ -67,9 +67,14 @@ * <code>retry.backoff.ms</code> */ public static final String RETRY_BACKOFF_MS_CONFIG = CommonClientConfigs.RETRY_BACKOFF_MS_CONFIG; - private static final String RETRY_BACKOFF_MS_DOC = "The amount of time to wait before attempting to " + - "retry a failed request. This avoids repeatedly sending requests in a tight loop under " + - "some failure scenarios."; + private static final String RETRY_BACKOFF_MS_DOC = CommonClientConfigs.RETRY_BACKOFF_MS_DOC; + + /** + * <code>retry.backoff.max.ms</code> + */ + // TODO: Add validator rules and force backoff_max_ms > backoff_ms if possible (I guess it's impossible) Review comment: Could we create JIRA instead of todo? ########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... + * The formula is Term(n) = random(1 - jitter, 1 + jitter) * scaleFactor * (ratio) ^ n + * If scaleFactor is greater or equal than termMax, a constant term of will be provided + * This class is thread-safe + */ +public class GeometricProgression { + private final int ratio; + private final double expMax; + private final long scaleFactor; + private final double jitter; + + public GeometricProgression(long scaleFactor, int ratio, long termMax, double jitter) { + this.scaleFactor = scaleFactor; + this.ratio = ratio; + this.jitter = jitter; + this.expMax = termMax > scaleFactor ? Review comment: Since all the users of this class would apply exponential back-off, I don't think maintaining the capability for state back-off is necessary to overload the class. ########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... Review comment: nit: An util class for... -> Utility class ########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... + * The formula is Term(n) = random(1 - jitter, 1 + jitter) * scaleFactor * (ratio) ^ n + * If scaleFactor is greater or equal than termMax, a constant term of will be provided Review comment: period ########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... + * The formula is Term(n) = random(1 - jitter, 1 + jitter) * scaleFactor * (ratio) ^ n + * If scaleFactor is greater or equal than termMax, a constant term of will be provided Review comment: equal to ########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... + * The formula is Term(n) = random(1 - jitter, 1 + jitter) * scaleFactor * (ratio) ^ n + * If scaleFactor is greater or equal than termMax, a constant term of will be provided + * This class is thread-safe + */ +public class GeometricProgression { Review comment: The classname and fields are too general, let's name them something directly relating to retry back-off IMHO. ########## File path: clients/src/main/java/org/apache/kafka/common/utils/GeometricProgression.java ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.kafka.common.utils; + +import java.util.concurrent.ThreadLocalRandom; + +/** + * An util class for exponential backoff, backoff, etc... + * The formula is Term(n) = random(1 - jitter, 1 + jitter) * scaleFactor * (ratio) ^ n + * If scaleFactor is greater or equal than termMax, a constant term of will be provided + * This class is thread-safe + */ +public class GeometricProgression { + private final int ratio; + private final double expMax; + private final long scaleFactor; Review comment: This is a little bit hard to understand, should we name it `baseBackOffMs` ---------------------------------------------------------------- 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: us...@infra.apache.org
