You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "dajac (via GitHub)" <gi...@apache.org> on 2024/05/16 12:51:55 UTC
[PR] MINOR: Refactor write timeout in CoordinatorRuntime [kafka]
dajac opened a new pull request, #15976:
URL: https://github.com/apache/kafka/pull/15976
This patch is a small refactor in the CoordinatorRuntime. It relies on existing tests for correctness.
### 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: Refactor write timeout in CoordinatorRuntime [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #15976:
URL: https://github.com/apache/kafka/pull/15976#discussion_r1603398900
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -541,6 +541,28 @@ private void unload() {
}
}
+ class OperationTimeout extends TimerTask {
+ private final TopicPartition tp;
+ private final DeferredEvent event;
+
+ public OperationTimeout(
+ TopicPartition tp,
+ DeferredEvent event,
+ long delayMs
+ ) {
+ super(delayMs);
+ this.event = event;
+ this.tp = tp;
+ }
+
+ @Override
+ public void run() {
+ String name = event.toString();
+ scheduleInternalOperation("OperationTimeout", tp,
Review Comment:
Done.
--
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: Refactor write timeout in CoordinatorRuntime [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #15976:
URL: https://github.com/apache/kafka/pull/15976#discussion_r1603394774
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -541,6 +541,28 @@ private void unload() {
}
}
+ class OperationTimeout extends TimerTask {
+ private final TopicPartition tp;
+ private final DeferredEvent event;
+
+ public OperationTimeout(
+ TopicPartition tp,
+ DeferredEvent event,
+ long delayMs
+ ) {
+ super(delayMs);
+ this.event = event;
+ this.tp = tp;
+ }
+
+ @Override
+ public void run() {
+ String name = event.toString();
Review Comment:
We don't need it anymore because we cancel the timer when the operation is completed. The future is completed at the same time so it is equivalent.
--
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: Refactor write timeout in CoordinatorRuntime [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15976:
URL: https://github.com/apache/kafka/pull/15976#discussion_r1603390406
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -541,6 +541,28 @@ private void unload() {
}
}
+ class OperationTimeout extends TimerTask {
+ private final TopicPartition tp;
+ private final DeferredEvent event;
+
+ public OperationTimeout(
+ TopicPartition tp,
+ DeferredEvent event,
+ long delayMs
+ ) {
+ super(delayMs);
+ this.event = event;
+ this.tp = tp;
+ }
+
+ @Override
+ public void run() {
+ String name = event.toString();
+ scheduleInternalOperation("OperationTimeout", tp,
Review Comment:
previous operation name carries more information. Should we keep it? for instance: `"OperationTimeout(name=" + name + ", tp=" + tp + ")"`
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -541,6 +541,28 @@ private void unload() {
}
}
+ class OperationTimeout extends TimerTask {
+ private final TopicPartition tp;
+ private final DeferredEvent event;
+
+ public OperationTimeout(
+ TopicPartition tp,
+ DeferredEvent event,
+ long delayMs
+ ) {
+ super(delayMs);
+ this.event = event;
+ this.tp = tp;
+ }
+
+ @Override
+ public void run() {
+ String name = event.toString();
Review Comment:
It does not include `if (!future.isDone()) {` now. Is it ok? I don't have much context :)
--
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: Refactor write timeout in CoordinatorRuntime [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #15976:
URL: https://github.com/apache/kafka/pull/15976#discussion_r1603395535
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -541,6 +541,28 @@ private void unload() {
}
}
+ class OperationTimeout extends TimerTask {
+ private final TopicPartition tp;
+ private final DeferredEvent event;
+
+ public OperationTimeout(
+ TopicPartition tp,
+ DeferredEvent event,
+ long delayMs
+ ) {
+ super(delayMs);
+ this.event = event;
+ this.tp = tp;
+ }
+
+ @Override
+ public void run() {
+ String name = event.toString();
+ scheduleInternalOperation("OperationTimeout", tp,
Review Comment:
Let me add it back.
--
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: Refactor write timeout in CoordinatorRuntime [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #15976:
URL: https://github.com/apache/kafka/pull/15976
--
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