Copilot commented on code in PR #3860: URL: https://github.com/apache/hertzbeat/pull/3860#discussion_r2542359798
########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/support/exception/CommonDataQueueUnknownException.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.hertzbeat.common.support.exception; + +/** + * Common data queue unknown exception + */ +public class CommonDataQueueUnknownException extends RuntimeException { + + public CommonDataQueueUnknownException() { + } + + public CommonDataQueueUnknownException(String message) { + super(message); + } Review Comment: The exception class is missing constructors that accept a `Throwable` cause. This prevents callers from properly chaining exceptions and preserving stack traces. Add the following constructors: ```java public CommonDataQueueUnknownException(String message, Throwable cause) { super(message, cause); } public CommonDataQueueUnknownException(Throwable cause) { super(cause); } ``` These are standard constructors for custom exceptions and are needed for proper exception handling. ```suggestion } public CommonDataQueueUnknownException(String message, Throwable cause) { super(message, cause); } public CommonDataQueueUnknownException(Throwable cause) { super(cause); } ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/ExponentialBackoff.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.hertzbeat.common.util; + +/** + * Exponential backoff utility class + */ +public final class ExponentialBackoff { + private final long initial; + private final long max; + private long current; + + public ExponentialBackoff(long initial, long max) { + if (initial <= 0 || max < initial) { + throw new IllegalArgumentException("Invalid exponential backoff params"); + } + this.initial = initial; + this.max = max; + this.current = initial; + } + + public long nextDelay() { + long delay = this.current; + if (this.current <= this.max / 2) { + this.current = this.current * 2; + } else { + this.current = this.max; + } + return delay; + } + + public void reset() { + if (this.current != this.initial) { + this.current = this.initial; + } Review Comment: The condition `if (this.current != this.initial)` is unnecessary. The assignment `this.current = this.initial` is idempotent and can be done unconditionally without impacting correctness or performance. This simplifies the code: ```java public void reset() { this.current = this.initial; } ``` ```suggestion this.current = this.initial; ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/ExponentialBackoff.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.hertzbeat.common.util; + +/** + * Exponential backoff utility class + */ +public final class ExponentialBackoff { + private final long initial; + private final long max; + private long current; + + public ExponentialBackoff(long initial, long max) { + if (initial <= 0 || max < initial) { + throw new IllegalArgumentException("Invalid exponential backoff params"); + } + this.initial = initial; + this.max = max; + this.current = initial; + } + + public long nextDelay() { + long delay = this.current; + if (this.current <= this.max / 2) { + this.current = this.current * 2; + } else { + this.current = this.max; + } + return delay; + } + Review Comment: Add method documentation: ```java /** * Resets the backoff to the initial delay value. * Should be called when an operation succeeds after previous failures. */ ``` ```suggestion /** * Resets the backoff to the initial delay value. * Should be called when an operation succeeds after previous failures. */ ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/queue/impl/KafkaCommonDataQueue.java: ########## @@ -176,6 +177,7 @@ public <T> T genericPollDataFunction(LinkedBlockingQueue<T> dataQueue, KafkaCons dataConsumer.commitAsync(); } catch (Exception e) { log.error(e.getMessage()); + throw new CommonDataQueueUnknownException(); Review Comment: The `CommonDataQueueUnknownException` is thrown without including the original exception cause or message. This makes debugging difficult as the original error information is lost. Consider passing the original exception: ```java throw new CommonDataQueueUnknownException(e.getMessage(), e); ``` You'll also need to add a constructor to `CommonDataQueueUnknownException` that accepts a message and cause. ```suggestion throw new CommonDataQueueUnknownException(e.getMessage(), e); ``` ########## hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/DataStorageDispatch.java: ########## @@ -84,6 +90,15 @@ protected void startPersistentDataStorage() { } } catch (InterruptedException interruptedException) { Thread.currentThread().interrupt(); + } catch (CommonDataQueueUnknownException ue) { + if (Thread.currentThread().isInterrupted()) { + break; + } + try { + TimeUnit.MILLISECONDS.sleep(backoff.nextDelay()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } Review Comment: The exponential backoff logic is duplicated across multiple exception handlers in different methods and files. Consider extracting this retry pattern into a reusable helper method to reduce code duplication and improve maintainability. For example: ```java private void handleQueueExceptionWithBackoff(ExponentialBackoff backoff) { if (Thread.currentThread().isInterrupted()) { return; } try { TimeUnit.MILLISECONDS.sleep(backoff.nextDelay()); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } } ``` This pattern appears in at least 5 different locations across the codebase. ########## hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/DataStorageDispatch.java: ########## @@ -18,6 +18,8 @@ package org.apache.hertzbeat.warehouse.store; import java.util.Optional; +import java.util.concurrent.TimeUnit; + Review Comment: The blank line after the `TimeUnit` import creates inconsistent import grouping. According to Java style conventions, imports should be grouped (java.*, javax.*, then third-party), with blank lines between groups but not within groups. The `TimeUnit` import should not have a blank line after it since it's in the same group as other java.util imports that may exist. ```suggestion ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/config/CommonProperties.java: ########## @@ -125,6 +125,11 @@ public static class RedisProperties { */ private String logEntryToStorageQueueName; + /** + * Timeout for blocking wait (seconds) Review Comment: The documentation for `waitTimeout` should clarify what happens when this value is null or not configured, since the code defaults to 1L. Consider updating the comment to: ```java /** * Timeout for blocking wait in seconds (defaults to 1 second if not configured) */ ``` This helps users understand the default behavior. ```suggestion * Timeout for blocking wait in seconds (defaults to 1 second if not configured) ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/queue/impl/RedisCommonDataQueue.java: ########## @@ -179,7 +183,7 @@ private <T> T genericBlockingPollFunction(String key, RedisCommands<String, T> c } } catch (Exception e) { log.error("Redis BRPOP failed: {}", e.getMessage()); - return null; + throw new CommonDataQueueUnknownException(); Review Comment: The `CommonDataQueueUnknownException` is thrown without including the original exception cause or message. This makes debugging difficult as the original error information is lost. Consider passing the original exception: ```java throw new CommonDataQueueUnknownException(e.getMessage(), e); ``` You'll also need to add a constructor to `CommonDataQueueUnknownException` that accepts a message and cause. ```suggestion throw new CommonDataQueueUnknownException(e.getMessage(), e); ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/ExponentialBackoff.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.hertzbeat.common.util; + +/** + * Exponential backoff utility class Review Comment: The class-level documentation should explain the backoff behavior, including: - How the delay progression works (doubles each time until max) - When to call `reset()` - Thread-safety considerations (this class is not thread-safe) Example: ```java /** * Exponential backoff utility class. * * Provides exponentially increasing delays starting from an initial value, * doubling with each call to nextDelay() until reaching the maximum value. * Call reset() to restart from the initial delay. * * Note: This class is not thread-safe. Each thread should use its own instance. */ ``` ```suggestion * Exponential backoff utility class. * * <p>Provides exponentially increasing delays starting from an initial value, * doubling with each call to {@link #nextDelay()} until reaching the maximum value. * Call {@link #reset()} to restart from the initial delay.</p> * * <p>Note: This class is <b>not</b> thread-safe. Each thread should use its own instance.</p> ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/OptionalUtil.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.hertzbeat.common.util; + +/** + * Optional util + */ +public class OptionalUtil { + + /** + * return obj if not null, else return defaultValue + * + * @param obj object to check + * @param defaultValue default value + * @param <T> type + * @return obj or defaultValue + */ + public static <T> T ofNullable(T obj, T defaultValue) { + return obj == null ? defaultValue : obj; + } + + + /** + * return obj if not null, else throw IllegalArgumentException + * + * @param obj object to check + * @param <T> type + * @return obj + * @throws IllegalArgumentException if obj is null + */ + public static <T> T of(T obj) throws IllegalArgumentException { + if (obj == null) { + throw new IllegalArgumentException("Param must not be null"); + } + return obj; + } + +} Review Comment: [nitpick] This utility class duplicates functionality already available in Java's standard library. Consider using `Objects.requireNonNullElse(obj, defaultValue)` instead of `ofNullable()` and `Objects.requireNonNull(obj, "message")` instead of `of()`. This reduces maintenance burden and leverages well-tested standard library methods. If you prefer to keep the utility class for consistency with the project's style, that's acceptable, but it's worth considering whether the additional abstraction adds value. ```suggestion ``` ########## hertzbeat-common/src/main/java/org/apache/hertzbeat/common/util/ExponentialBackoff.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.hertzbeat.common.util; + +/** + * Exponential backoff utility class + */ +public final class ExponentialBackoff { + private final long initial; + private final long max; + private long current; + + public ExponentialBackoff(long initial, long max) { + if (initial <= 0 || max < initial) { + throw new IllegalArgumentException("Invalid exponential backoff params"); + } + this.initial = initial; + this.max = max; + this.current = initial; + } + Review Comment: Add method documentation explaining the behavior: ```java /** * Returns the current delay value and advances to the next delay. * The delay doubles on each call until it reaches the maximum value. * * @return the delay in milliseconds to wait before the next retry */ ``` ```suggestion /** * Returns the current delay value and advances to the next delay. * The delay doubles on each call until it reaches the maximum value. * * @return the delay in milliseconds to wait before the next retry */ ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
