chia7712 commented on code in PR #15710:
URL: https://github.com/apache/kafka/pull/15710#discussion_r1562828732
##########
server-common/src/test/java/org/apache/kafka/timeline/SnapshotRegistryTest.java:
##########
@@ -41,9 +42,7 @@ public void testEmptyRegistry() {
private static void assertIteratorContains(Iterator<Snapshot> iter,
Snapshot... snapshots) {
List<Snapshot> expected = new ArrayList<>();
Review Comment:
How about `List<Snapshot> expected = Arrays.asList(snapshots);`?
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsInstance.java:
##########
@@ -116,8 +116,7 @@ public synchronized boolean
maybeUpdatePushRequestTimestamp(long currentTime) {
*/
boolean canAccept = lastGetRequestTimestamp > lastPushRequestTimestamp;
Review Comment:
how about `boolean canAccept = lastGetRequestTimestamp >
lastPushRequestTimestamp || currentTime - lastPushRequestTimestamp >=
pushIntervalMs;`?
##########
server-common/src/test/java/org/apache/kafka/timeline/SnapshottableHashTableTest.java:
##########
@@ -310,10 +309,8 @@ private static void assertIteratorYields(Iterator<?> iter,
}
}
if (!extraObjects.isEmpty() || !remaining.isEmpty()) {
- throw new RuntimeException("Found extra object(s): [" +
String.join(", ",
- extraObjects.stream().map(e ->
e.toString()).collect(Collectors.toList())) +
- "] and didn't find object(s): [" + String.join(", ",
- remaining.keySet().stream().map(e ->
e.toString()).collect(Collectors.toList())) + "]");
+ throw new RuntimeException("Found extra object(s): [" +
extraObjects.stream().map(e -> e.toString()).collect(Collectors.joining(", ")) +
Review Comment:
`e -> e.toString()` -> `Object::toString`
##########
server/src/main/java/org/apache/kafka/server/AssignmentsManager.java:
##########
@@ -321,7 +321,7 @@ public void run() throws Exception {
AssignReplicasToDirsResponseData data =
((AssignReplicasToDirsResponse) response.responseBody()).data();
Set<AssignmentEvent> failed = filterFailures(data, inflight);
- Set<AssignmentEvent> completed = Utils.diff(HashSet::new,
inflight.values().stream().collect(Collectors.toSet()), failed);
+ Set<AssignmentEvent> completed = Utils.diff(HashSet::new, new
HashSet<>(inflight.values()), failed);
Review Comment:
Maybe we don't need to create collection many times by skipping the failed
elements.
```java
Set<AssignmentEvent> failed = filterFailures(data, inflight);
for (AssignmentEvent assignmentEvent : inflight.values()) {
if (failed.contains(assignmentEvent)) continue;
if (log.isDebugEnabled()) {
log.debug("Successfully propagated assignment {}",
assignmentEvent);
}
assignmentEvent.onComplete();
}
```
--
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
