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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
