Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

mimaison merged PR #15705:
URL: https://github.com/apache/kafka/pull/15705


-- 
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

Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

mimaison commented on PR #15705:
URL: https://github.com/apache/kafka/pull/15705#issuecomment-2059036435

   None of the test failures seem related, merging to trunk.


-- 
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

Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

mimaison commented on code in PR #15705:
URL: https://github.com/apache/kafka/pull/15705#discussion_r1565764251


##
clients/src/main/java/org/apache/kafka/clients/admin/internals/CoordinatorStrategy.java:
##
@@ -136,8 +136,8 @@ private CoordinatorKey 
requireSingletonAndType(Set keys) {
 }
 
 private void ensureSameType(Set 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:
   It's the same really. I just simplified the error message but be a fixed 
string.



-- 
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

Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

mimaison commented on code in PR #15705:
URL: https://github.com/apache/kafka/pull/15705#discussion_r1565721043


##
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 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 bufferQueue = 
bufferMap.computeIfAbsent(buffer.capacity(), k -> new ArrayDeque<>(1));
+// We currently keep a single buffer in flight, so optimise for 
that case

Review Comment:
   Good point!



-- 
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

Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

mimaison commented on code in PR #15705:
URL: https://github.com/apache/kafka/pull/15705#discussion_r1565719621


##
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 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:
   Yes I agree



-- 
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

Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

mimaison commented on code in PR #15705:
URL: https://github.com/apache/kafka/pull/15705#discussion_r1565718889


##
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:
   Since this field is unused so I thought we should just delete it.



-- 
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

Re: [PR] MINOR: Various cleanups in clients [kafka]

[via GitHub]

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 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 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 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 keys) {
 }
 
 private void ensureSameType(Set 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