You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2024/01/18 21:46:48 UTC
[PR] KAFKA-16166: Generify RetryWithToleranceOperator and ErrorReporter classes [kafka]
gharris1727 opened a new pull request, #15233:
URL: https://github.com/apache/kafka/pull/15233
This is a follow-up to #15154 which propagates the generic type for ProcessingContext upward through all its call-sites.
I decided to avoid generifying the WorkerTask and instead separate the common retryWithToleranceOperator field into two fields with different types, in WorkerSinkTask and AbstractWorkerSourceTask, as the only operation which was present in the WorkerTask was the retryWithToleranceOperator.triggerStop() call.
This now makes the type system enforce that the DeadLetterQueueReporter only works for sink tasks, and the LogReporter must be specialized as either LogReporter.Sink or LogReporter.Source.
### 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-16166: Generify RetryWithToleranceOperator and ErrorReporter classes [kafka]
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #15233:
URL: https://github.com/apache/kafka/pull/15233#discussion_r1459683206
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java:
##########
@@ -132,7 +127,6 @@ public void stop() {
*/
public void cancel() {
cancelled = true;
- retryWithToleranceOperator.triggerStop();
Review Comment:
I know it's a little strange for this class to hold onto the `RetryWithToleranceOperator` when its only interaction is to stop it, but this seemed reasonable in https://github.com/apache/kafka/pull/12478 because it saved us the potential headache of adding another subclass to `WorkerTask` that might forget to invoke `RetryWithToleranceOperator::triggerStop`.
I'm not hugely opposed to changing this part and it's not a blocker, but FWIW I think this change is unnecessary.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/DeadLetterQueueReporter.java:
##########
@@ -124,19 +124,17 @@ public static DeadLetterQueueReporter createAndSetup(Map<String, Object> adminPr
* @param context processing context containing the raw record at {@link ProcessingContext#original()}.
* @return the future associated with the writing of this record; never null
*/
- @SuppressWarnings("unchecked")
Review Comment:
🙌
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/TransformationChain.java:
##########
@@ -32,18 +32,18 @@
* Represents a chain of {@link Transformation}s to be applied to a {@link ConnectRecord} serially.
* @param <R> The type of record (must be an implementation of {@link ConnectRecord})
*/
-public class TransformationChain<R extends ConnectRecord<R>> implements AutoCloseable {
+public class TransformationChain<T, R extends ConnectRecord<R>> implements AutoCloseable {
Review Comment:
Can we update the Javadocs right above with a description of the new type parameter?
--
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-16166: Generify RetryWithToleranceOperator and ErrorReporter classes [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #15233:
URL: https://github.com/apache/kafka/pull/15233#discussion_r1459984454
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java:
##########
@@ -132,7 +127,6 @@ public void stop() {
*/
public void cancel() {
cancelled = true;
- retryWithToleranceOperator.triggerStop();
Review Comment:
I've added a commit that generifies WorkerTask, so that this can remain a field there in the superclass. I also moved up the error reporter supplier and transformation chain that were in a similar situation. Let me know what you think of those changes :)
--
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-16166: Generify RetryWithToleranceOperator and ErrorReporter classes [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #15233:
URL: https://github.com/apache/kafka/pull/15233#issuecomment-1905100656
Test failures appear unrelated, and the runtime tests pass for me locally.
--
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-16166: Generify RetryWithToleranceOperator and ErrorReporter classes [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 merged PR #15233:
URL: https://github.com/apache/kafka/pull/15233
--
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