You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mimaison (via GitHub)" <gi...@apache.org> on 2024/04/12 15:30:51 UTC
[PR] Various cleanups in server and server-common [kafka]
mimaison opened a new pull request, #15710:
URL: https://github.com/apache/kafka/pull/15710
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in server and server-common [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #15710:
URL: https://github.com/apache/kafka/pull/15710#discussion_r1565856071
##########
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:
I'd prefer only sticking to simple refactorings in this PR. We can do this optimization in another PR.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in server and server-common [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #15710:
URL: https://github.com/apache/kafka/pull/15710#discussion_r1565897312
##########
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:
It's shorter but I'm not sure it's more readable.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in server and server-common [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #15710:
URL: https://github.com/apache/kafka/pull/15710
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Various cleanups in server and server-common [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org