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 2023/04/13 09:09:32 UTC
[GitHub] [kafka] dajac opened a new pull request, #13555: MINOR: Move `ControllerPurgatory` to `server-common`
dajac opened a new pull request, #13555:
URL: https://github.com/apache/kafka/pull/13555
I would like to reuse `ControllerPurgatory` class in the new group coordinator for the exact same need. This patch moves it `server-common` and renames it to `Purgatory`. I use `org.apache.kafka.purgatory` as package name to be like `org.apache.kafka.queue`.
### 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
[GitHub] [kafka] dajac merged pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #13555:
URL: https://github.com/apache/kafka/pull/13555
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172674549
##########
server-common/src/main/java/org/apache/kafka/purgatory/Purgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
- * We wait for the high water mark of the metadata log to advance before completing
- * them.
+ * We wait for the high watermark of the log to advance before completing them.
*/
-class ControllerPurgatory {
+public class Purgatory {
Review Comment:
+1 for `DeferredEventPurgatory`.
--
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
[GitHub] [kafka] dengziming commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dengziming (via GitHub)" <gi...@apache.org>.
dengziming commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1173247022
##########
server-common/src/main/java/org/apache/kafka/purgatory/DeferredEventPurgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
Review Comment:
Some of my thoughts: the founder of Kafka had some artistic elements, with Kafka(a novelist) mainly representing the modernist school, and "purgatory" being a term used in Dante's "Divine Comedy", a representative writer of the Renaissance era. Purgatory' is first used in the `TimingWheel` algorithm and has been used several times to represent a `ThresholdFuture` data structure.
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172738445
##########
server-common/src/main/java/org/apache/kafka/purgatory/DeferredEventPurgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
- * We wait for the high water mark of the metadata log to advance before completing
- * them.
+ * We wait for the high watermark of the log to advance before completing them.
*/
-class ControllerPurgatory {
+public class DeferredEventPurgatory {
Review Comment:
Naming things is hard :). `Completion` sounds a bit weird here. I can't really think of a better name...
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172735940
##########
server-common/src/main/java/org/apache/kafka/purgatory/DeferredEventPurgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
Review Comment:
Why do you think that it is not accurate? For me, a purgatory is a data structure holding something which is completed later. That seems to fit 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
[GitHub] [kafka] dengziming commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dengziming (via GitHub)" <gi...@apache.org>.
dengziming commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172598460
##########
server-common/src/main/java/org/apache/kafka/purgatory/Purgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
- * We wait for the high water mark of the metadata log to advance before completing
- * them.
+ * We wait for the high watermark of the log to advance before completing them.
*/
-class ControllerPurgatory {
+public class Purgatory {
Review Comment:
It seems better to add a prefix since we have several `Purgatory` classes, we have a `FuturePurgatory` in raft module and its similar to this `ControllerPurgatory`, we may call it `DeferredEventPurgatory` or HwmPurgatory(high watermark).
--
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
[GitHub] [kafka] jsancio commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172839235
##########
server-common/src/main/java/org/apache/kafka/deferred/DeferredEvent.java:
##########
@@ -15,12 +15,12 @@
* limitations under the License.
*/
-package org.apache.kafka.controller;
+package org.apache.kafka.deferred;
/**
* Represents a deferred event in the controller purgatory.
Review Comment:
Let's fix this comment. Maybe:
> Represents a deferred event in the {@code DeferredEventQueue}.
##########
server-common/src/main/java/org/apache/kafka/deferred/DeferredEventQueue.java:
##########
@@ -77,13 +76,13 @@ void failAll(Exception exception) {
* @param offset The offset to add the new event at.
Review Comment:
Above we have:
> Add a new purgatory event.
how about:
> Add a new deferred event to be completed by the provided offset.
##########
server-common/src/main/java/org/apache/kafka/deferred/DeferredEventQueue.java:
##########
@@ -77,13 +76,13 @@ void failAll(Exception exception) {
* @param offset The offset to add the new event at.
* @param event The new event.
*/
- void add(long offset, DeferredEvent event) {
+ public void add(long offset, DeferredEvent event) {
if (!pending.isEmpty()) {
long lastKey = pending.lastKey();
if (offset < lastKey) {
throw new RuntimeException("There is already a purgatory event with " +
Review Comment:
How about throwing an `IllegalArgumentException` instead of `RuntimeException` with the following message:
> There is already a deferred event with offset ...
##########
server-common/src/main/java/org/apache/kafka/deferred/DeferredEventQueue.java:
##########
@@ -60,7 +59,7 @@ void completeUpTo(long offset) {
*
Review Comment:
The line above has
> Fail all the pending purgatory entries.
how about:
> Fail all deferred events with the provided exception.
--
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
[GitHub] [kafka] jsancio commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172724317
##########
server-common/src/main/java/org/apache/kafka/purgatory/DeferredEventPurgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
Review Comment:
Minor but can we avoid the use of the word purgatory? It is not accurate for what this type and package are doing.
##########
server-common/src/main/java/org/apache/kafka/purgatory/DeferredEventPurgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
- * We wait for the high water mark of the metadata log to advance before completing
- * them.
+ * We wait for the high watermark of the log to advance before completing them.
*/
-class ControllerPurgatory {
+public class DeferredEventPurgatory {
Review Comment:
Maybe a more accurate name could be `OffsetBasedCompletion`
--
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
[GitHub] [kafka] jsancio commented on a diff in pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13555:
URL: https://github.com/apache/kafka/pull/13555#discussion_r1172793737
##########
server-common/src/main/java/org/apache/kafka/purgatory/DeferredEventPurgatory.java:
##########
@@ -26,10 +26,9 @@
/**
* The purgatory which holds events that have been started, but not yet completed.
Review Comment:
This is Merriam Webster definition:
> 1 : an intermediate state after death for expiatory purification specifically : a place or state of punishment wherein according to Roman Catholic doctrine the souls of those who die in God's grace may make satisfaction for past sins and so become fit for heaven
> 2 : a place or state of temporary suffering or misery
I don't think either of those definition is accurate for this problem and solution.
--
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
[GitHub] [kafka] dajac commented on pull request #13555: MINOR: Move `ControllerPurgatory` to `server-common`
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #13555:
URL: https://github.com/apache/kafka/pull/13555#issuecomment-1506619381
@cmccabe Could you take 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