You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mwesterby (via GitHub)" <gi...@apache.org> on 2023/12/06 15:17:52 UTC
[PR] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
mwesterby opened a new pull request, #14942:
URL: https://github.com/apache/kafka/pull/14942
Adds a new metric, QueuedReplicaToDirAssignments, which, when queried, will return the total number of replica to directory assignments that are currently queued in the AssignmentsManager.
### 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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-1850876018
@mwesterby Can you resolve the conflicts here?
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-1850990787
@mwesterby Looks like we have a checkstyle issue:
```
Task :server:checkstyleTest
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14942/server/src/test/java/org/apache/kafka/server/AssignmentsManagerTest.java:336:81: '{' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14942/server/src/test/java/org/apache/kafka/server/AssignmentsManagerTest.java:336:82: '}' is not preceded with whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14942/server/src/test/java/org/apache/kafka/server/AssignmentsManagerTest.java:344:81: '{' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14942/server/src/test/java/org/apache/kafka/server/AssignmentsManagerTest.java:344:82: '}' is not preceded with whitespace. [WhitespaceAround]
> Task :server:checkstyleTest FAILED
```
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-1851499573
LGTM, failing tests are unrelated. Merging.
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "Saboteurs09 (via GitHub)" <gi...@apache.org>.
Saboteurs09 commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-3868314425
## PR Gatekeeper — Final Review
| **Risk Score** | **Decision** |
|----------------|-------------|
| **13** / 100 | **APPROVE** |
### Summary
This PR is approved for merge from a risk perspective.
### Key risks
- **No AI insights** — AI review was skipped or returned no insights.
### Test execution
- **Passed:** 0 / 0
- **Time:** 0 ms
---
*This review was generated by the PR Gatekeeper. Please address risks before merging.*
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "Saboteurs09 (via GitHub)" <gi...@apache.org>.
Saboteurs09 commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-3868281591
## PR Gatekeeper — Final Review
| **Risk Score** | **Decision** |
|----------------|-------------|
| **18** / 100 | **APPROVE** |
### Summary
This PR is approved for merge from a risk perspective.
### Key risks
- **No AI insights** — AI review was skipped or returned no insights.
### Test gaps
- `server/src/main/java/org/apache/kafka/server/AssignmentsManager.java` — no corresponding test class (suggested: `src/test/java/server/src/main/java/org/apache/kafka/server/AssignmentsManager.java`)
### Test execution
- **Passed:** 0 / 0
- **Time:** 0 ms
---
*This review was generated by the PR Gatekeeper. Please address risks before merging.*
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "mwesterby (via GitHub)" <gi...@apache.org>.
mwesterby commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-1851003645
Should be fixed now!
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "Saboteurs09 (via GitHub)" <gi...@apache.org>.
Saboteurs09 commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-3868327345
## PR Gatekeeper — Final Review
| **Risk Score** | **Decision** |
|----------------|-------------|
| **13** / 100 | **APPROVE** |
### Summary
This PR is approved for merge from a risk perspective.
### Key risks
- **No AI insights** — AI review was skipped or returned no insights.
### Test execution
- **Passed:** 0 / 0
- **Time:** 0 ms
---
*This review was generated by the PR Gatekeeper. Please address risks before merging.*
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #14942:
URL: https://github.com/apache/kafka/pull/14942#discussion_r1423140834
##########
server/src/main/java/org/apache/kafka/server/AssignmentsManager.java:
##########
@@ -93,10 +100,24 @@ public AssignmentsManager(Time time,
"broker-" + brokerId + "-directory-assignments-manager-",
new ShutdownEvent());
channelManager.start();
+ this.metricsGroup.newGauge(QUEUE_REPLICA_TO_DIR_ASSIGNMENTS_METRIC_NAME, new Gauge<Integer>() {
+ @Override
+ public Integer value() {
+ return getMapSize(inflight) + getMapSize(pending);
Review Comment:
The map references are copied into `getMapSize` which avoids a check-then-act NPE race.
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-1843100363
@rondagostino @cmccabe @pprovenzano please have a look
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "Saboteurs09 (via GitHub)" <gi...@apache.org>.
Saboteurs09 commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-3868300654
## PR Gatekeeper — Final Review
| **Risk Score** | **Decision** |
|----------------|-------------|
| **13** / 100 | **APPROVE** |
### Summary
This PR is approved for merge from a risk perspective.
### Key risks
- **No AI insights** — AI review was skipped or returned no insights.
### Test gaps
- `server/src/main/java/org/apache/kafka/server/AssignmentsManager.java` (suggested: `server/src/test/java/org/apache/kafka/server/AssignmentsManagerTest.java`)
### Test execution
- **Passed:** 0 / 0
- **Time:** 0 ms
---
*This review was generated by the PR Gatekeeper. Please address risks before merging.*
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on code in PR #14942:
URL: https://github.com/apache/kafka/pull/14942#discussion_r1422561158
##########
server/src/main/java/org/apache/kafka/server/AssignmentsManager.java:
##########
@@ -240,31 +269,53 @@ public void run() throws Exception {
}
}
+ private void updateQueuedReplicaAssignmentsMetric() {
+ int total = 0;
+ if (pending != null) total += pending.size();
+ if (inflight != null) total += inflight.size();
+ queuedReplicaAssignments.set(total);
+ }
+
/**
* Handles the response to a dispatched {@link AssignReplicasToDirsRequest}.
*/
private class AssignmentResponseEvent extends Event {
private final ClientResponse response;
+
public AssignmentResponseEvent(ClientResponse response) {
this.response = response;
}
+
@Override
public void run() throws Exception {
+ try {
+ handleResponse();
+ } catch (Throwable e) {
+ log.error("Error handling assignment response", e);
+ } finally {
+ updateQueuedReplicaAssignmentsMetric();
+ }
+ }
+
+ void handleResponse() {
if (inflight == null) {
- throw new IllegalStateException("Bug. Cannot not be handling a client response if there is are no assignments in flight");
+ throw new IllegalStateException("Bug. Cannot not be handling a client response " +
+ "if there is are no assignments in flight");
}
if (responseIsError(response)) {
requeueAllAfterFailure();
} else {
failedAttempts = 0;
- AssignReplicasToDirsResponseData data = ((AssignReplicasToDirsResponse) response.responseBody()).data();
+ 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,
+ inflight.values().stream().collect(Collectors.toSet()), failed);
completed.forEach(assignmentEvent -> assignmentEvent.callback.run());
+ log.warn("Re-queueing assignments: {}", failed);
if (!failed.isEmpty()) {
- log.warn("Re-queueing assignments: {}", failed);
Review Comment:
nit: seems odd to log a warning even for the case where the set is 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #14942:
URL: https://github.com/apache/kafka/pull/14942#issuecomment-1850488367
Can we get the conflicts resolved?
--
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] KAFKA-15358: Added QueuedReplicaToDirAssignments metric [kafka]
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino merged PR #14942:
URL: https://github.com/apache/kafka/pull/14942
--
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