Copilot commented on code in PR #16661:
URL: https://github.com/apache/pinot/pull/16661#discussion_r2292233770
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BrokerRequestIdGenerator.java:
##########
@@ -18,32 +18,26 @@
*/
package org.apache.pinot.broker.requesthandler;
+import java.util.Random;
import java.util.concurrent.atomic.AtomicLong;
-/**
- * An ID generator to produce a global unique identifier for each query, used
in v1/v2 engine for tracking and
- * inter-stage communication(v2 only). It's guaranteed by:
- * <ol>
- * <li>
- * Using a mask computed using the hash-code of the broker-id to ensure
two brokers don't arrive at the same
- * requestId. This mask becomes the most significant 9 digits (in base-10).
- * </li>
- * <li>
- * Using a auto-incrementing counter for the least significant 9 digits
(in base-10).
- * </li>
- * </ol>
- */
+
+/// An ID generator to produce a global unique identifier for each query, used
in single-stage/multi-stage engine for
+/// tracking and inter-stage communication (multi-stage engine only). It's
guaranteed by:
+/// - Using a random number to ensure two brokers (or restarted broker) don't
arrive at the same request ID. This random
+/// number becomes the most significant 10 digits of the request ID (in
base-10).
+/// - Using an auto-incrementing counter for the least significant 9 digits
(in base-10).
public class BrokerRequestIdGenerator {
private static final long OFFSET = 1_000_000_000L;
- private final long _mask;
+ private final long _base;
private final AtomicLong _incrementingId = new AtomicLong(0);
- public BrokerRequestIdGenerator(String brokerId) {
- _mask = ((long) (brokerId.hashCode() & Integer.MAX_VALUE)) * OFFSET;
+ public BrokerRequestIdGenerator() {
+ _base = new Random().nextInt(Integer.MAX_VALUE) * OFFSET;
Review Comment:
Creating a new Random() instance on each BrokerRequestIdGenerator
instantiation is inefficient. Consider using a static Random instance or
ThreadLocalRandom for better performance.
```suggestion
_base = ThreadLocalRandom.current().nextInt(Integer.MAX_VALUE) * OFFSET;
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BrokerRequestIdGenerator.java:
##########
@@ -18,32 +18,26 @@
*/
package org.apache.pinot.broker.requesthandler;
+import java.util.Random;
import java.util.concurrent.atomic.AtomicLong;
-/**
- * An ID generator to produce a global unique identifier for each query, used
in v1/v2 engine for tracking and
- * inter-stage communication(v2 only). It's guaranteed by:
- * <ol>
- * <li>
- * Using a mask computed using the hash-code of the broker-id to ensure
two brokers don't arrive at the same
- * requestId. This mask becomes the most significant 9 digits (in base-10).
- * </li>
- * <li>
- * Using a auto-incrementing counter for the least significant 9 digits
(in base-10).
- * </li>
- * </ol>
- */
+
+/// An ID generator to produce a global unique identifier for each query, used
in single-stage/multi-stage engine for
+/// tracking and inter-stage communication (multi-stage engine only). It's
guaranteed by:
+/// - Using a random number to ensure two brokers (or restarted broker) don't
arrive at the same request ID. This random
+/// number becomes the most significant 10 digits of the request ID (in
base-10).
+/// - Using an auto-incrementing counter for the least significant 9 digits
(in base-10).
public class BrokerRequestIdGenerator {
private static final long OFFSET = 1_000_000_000L;
- private final long _mask;
+ private final long _base;
private final AtomicLong _incrementingId = new AtomicLong(0);
- public BrokerRequestIdGenerator(String brokerId) {
- _mask = ((long) (brokerId.hashCode() & Integer.MAX_VALUE)) * OFFSET;
+ public BrokerRequestIdGenerator() {
+ _base = new Random().nextInt(Integer.MAX_VALUE) * OFFSET;
Review Comment:
Using Random() without a seed makes the random number generation
predictable. Consider using SecureRandom or ThreadLocalRandom for better
cryptographic security, especially since this is used for request ID generation.
```suggestion
_base = new SecureRandom().nextInt(Integer.MAX_VALUE) * OFFSET;
```
--
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]
