chia7712 commented on code in PR #15705: URL: https://github.com/apache/kafka/pull/15705#discussion_r1562938471
########## clients/src/main/java/org/apache/kafka/common/memory/GarbageCollectedMemoryPool.java: ########## @@ -37,13 +37,13 @@ public class GarbageCollectedMemoryPool extends SimpleMemoryPool implements Auto //serves 2 purposes - 1st it maintains the ref objects reachable (which is a requirement for them //to ever be enqueued), 2nd keeps some (small) metadata for every buffer allocated private final Map<BufferReference, BufferMetadata> buffersInFlight = new ConcurrentHashMap<>(); - private final GarbageCollectionListener gcListener = new GarbageCollectionListener(); private final Thread gcListenerThread; - private volatile boolean alive = true; + private volatile boolean alive; public GarbageCollectedMemoryPool(long sizeBytes, int maxSingleAllocationSize, boolean strict, Sensor oomPeriodSensor) { super(sizeBytes, maxSingleAllocationSize, strict, oomPeriodSensor); this.alive = true; Review Comment: it seems to me removing `this.alive = true;` is more suitable. ########## clients/src/main/java/org/apache/kafka/common/utils/BufferSupplier.java: ########## @@ -79,12 +79,8 @@ public ByteBuffer get(int size) { @Override public void release(ByteBuffer buffer) { buffer.clear(); - Deque<ByteBuffer> bufferQueue = bufferMap.get(buffer.capacity()); - if (bufferQueue == null) { - // We currently keep a single buffer in flight, so optimise for that case - bufferQueue = new ArrayDeque<>(1); - bufferMap.put(buffer.capacity(), bufferQueue); - } + Deque<ByteBuffer> bufferQueue = bufferMap.computeIfAbsent(buffer.capacity(), k -> new ArrayDeque<>(1)); + // We currently keep a single buffer in flight, so optimise for that case Review Comment: Can we move this comment up? ########## clients/src/main/java/org/apache/kafka/common/network/SslChannelBuilder.java: ########## @@ -60,7 +58,6 @@ public SslChannelBuilder(Mode mode, this.mode = mode; this.listenerName = listenerName; this.isInterBrokerListener = isInterBrokerListener; - this.log = logContext.logger(getClass()); Review Comment: maybe we should pass it to `Utils.closeQuietly` instead of deleting it. ########## clients/src/main/java/org/apache/kafka/clients/admin/internals/CoordinatorStrategy.java: ########## @@ -136,8 +136,8 @@ private CoordinatorKey requireSingletonAndType(Set<CoordinatorKey> keys) { } private void ensureSameType(Set<CoordinatorKey> keys) { - if (keys.size() < 1) { - throw new IllegalArgumentException("Unexpected size of key set: expected >= 1, but got " + keys.size()); + if (keys.isEmpty()) { + throw new IllegalArgumentException("Unexpected size of key set: expected >= 1, but got 0"); Review Comment: maybe we can say "the keys can't be empty" -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org