feature: #242 Explicit terminal and retry exceptions for cleaner logging and poison pills #291

astubbs · 2022-05-06T12:12:34Z

convert output of some tests to use the new exception and check logging output is as expected
docs, including batch partial retry
tests
partial batch support - seperate PR
max retries handling
rewrite docs section: 11.2. Skipping Records
retry dlc PR

Retry system improvements #66

Related branches:

retry-dlq

...src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java

…eatures/retry-exception

astubbs · 2022-05-12T16:46:51Z

#268 ?

…xception

ifeeneyconfluent

FWIW, I'm happy to approve.

rkolesnev · 2022-05-13T14:29:28Z

...src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java

- log.error("Exception caught in user function running stage, registering WC as failed, returning to mailbox", e);
+ String msg = "Exception caught in user function running stage, registering WC as failed, returning to mailbox";
+ if (e instanceof RetriableException) {
+ log.debug("Explicit " + RetriableException.class.getSimpleName() + " caught, logging at DEBUG only. " + msg, e);


Suggested change

log.debug("Explicit " + RetriableException.class.getSimpleName() + " caught, logging at DEBUG only. " + msg, e);

log.debug("Explicit {} caught - {}", RetriableException.class.getSimpleName(), msg, e);

rkolesnev · 2022-05-13T14:39:15Z

...src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java

+ ParallelConsumerOptions.class.getSimpleName(),
+ context);
+
+ closeDontDrainFirst();


would it shutdown gracefully in this case? i.e. commit all that succeeded by this point, maybe should give some time for inflight processes as well (if it doesnt already) ? - to reduce possible duplicates on restart / rebalance.

yeah, it is graceful - closeDontDrainFirst will commit everything that's done first - drain isn't graceful, it means process everything in buffers.
Inflight - good point, not 100% will need to double check.

rkolesnev · 2022-05-13T14:40:13Z

...src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java

+ } catch (Exception e) {
+ log.error("Unknown internal error handling user function dispatch, terminating");
+
+ closeDontDrainFirst();
+
+ // throw again to make the future failed
+ throw e;
 }


what is current behaviour for user function exceptions ? is it a breaking change in behaviour?
I think i am missing the flow here - wouldn't this just kill PC and not retry the message?

current = retry :)

wouldn't this just kill PC and not retry the message?

yeah there was a bug - check latest version?

astubbs · 2022-05-17T18:07:57Z

Consider splitting the partial batch failure support into a seperate task.

…ption # Conflicts: # parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java # parallel-consumer-core/src/test/resources/logback-test.xml

astubbs

...

astubbs · 2022-05-20T14:54:51Z

parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/PCUserException.java

+ */
+
+/**
+ * todo


astubbs · 2022-05-20T14:58:02Z

...nsumer-core/src/test/java/io/confluent/parallelconsumer/internal/UserFunctionRunnerTest.java

+/**
+ * @see UserFunctionRunner
+ */
+class UserFunctionRunnerTest {


finish test

astubbs · 2022-05-20T14:58:11Z

parallel-consumer-core/src/test/resources/logback-test.xml

@@ -27,8 +27,8 @@
 </root>

 <!-- primary -->
- <logger name="io.confluent.parallelconsumer" level="info"/>
-<!-- <logger name="io.confluent.parallelconsumer" level="debug"/>-->
+ <logger name="io.confluent.parallelconsumer" level="debug"/>


rkolesnev · 2022-05-16T09:48:51Z

parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/PCTerminalException.java

+/**
+ * A user's processing function can throw this exception, which signals to PC that processing of the message has failed,
+ * and that it should be retired at a later time.
+ * <p>
+ * The advantage of throwing this exception explicitly, is that PC will not log an ERROR. If any other type of exception
+ * is thrown by the user's function, that will be logged as an error (but will still be retried later).
+ * <p>
+ * So in short, if this exception is thrown, nothing will be logged (except at DEBUG level), any other exception will be
+ * logged as an error.
+ */


update java doc

rkolesnev · 2022-05-16T10:20:37Z

...l-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/UserFunctionRunner.java

+
+@AllArgsConstructor
+@Slf4j
+public class UserFunctionRunner<K, V> {


Logic seems ok.
Not really sure about the new class and abstraction / concern separation - seems most of handling is calling back to PC - so the class seems like a utility / mixin type of class.
Not sure how it could be refactored better but now it seems adding more spaghetti-ness then making things cleaner - flow became PC-> UserFunctionRunner.runUserFunction -> PC with backward reference - that feels like a smell...
Would messaging / handling of execution result be part of the message based eventing / shared nothing refactor?
Alternatively could the success / failure notifications be moved back to PC and passed as callbacks?
Basically i think it would benefit from a bit of mapping of flows and rethinking of cleaner architecture / class split.

rkolesnev · 2022-05-16T10:27:25Z

...l-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/UserFunctionRunner.java

+ } catch (PCUserException e) {
+ // throw again to make the future failed
+ throw e;
+ } catch (Exception e) {
+ log.error("Unknown internal error handling user function dispatch, terminating");
+
+ pc.closeDontDrainFirst();
+
+ // throw again to make the future failed
+ throw e;
+ }


Not too keen on this - you are still killing PC on general Exception thrown from userFunction - effectively making default behaviour for non-specified exceptions a worst case one ( Terminal with Shutdown behaviour).
Did you mean to re-throw Exception as PCUserException on line 88?
Do you need a general Exception catch here at all - only other place that could throw is
Line 47 boolean workIsStale = pc.getWm().checkIfWorkIsStale(workContainerBatch);
could it handle exception inside that call instead - to leave here handling of userFunction call only?

This needs to be optional behaviour - I thought I had implemented that in this PR, but might be somewhere else. Either way, it should do like KS does - and have this be configurable.
I'll also probably copy their default behaviour.

rkolesnev · 2022-05-16T10:35:00Z

...l-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/UserFunctionRunner.java

+ /**
+ * Run the supplied function.
+ */
+ protected <R> List<Tuple<ConsumerRecord<K, V>, R>> runUserFunction(Function<PollContextInternal<K, V>, List<R>> usersFunction,


would it be better to change it to be a bit more readable?
ie. this would become something along the lines of

execute(){ try{ doDebugLogging(...); skipOnStaleWork(...); PollContextInternal<K, V> context = new PollContextInternal<>(workContainerBatch); runWithUserExceptions(...) }catch(...){ ... } }

If exception from stale work check is specific or handled in that call itself - can even get rid of inner runWithUserExceptions method and pull exception handling up here. esp given its the main purpose of this class...

eddyv · 2023-06-15T14:34:52Z

Closing - Stale.

nizarsalhaji94 · 2023-12-20T22:53:55Z

Hi @astubbs

Is a released date is planned ?
I’m interested to use SHUTDOWN Terminal Failure Reatction.
In fact, we need to shutdown (or stop or kill) the PC after max retry.

Thanks

astubbs · 2023-12-21T02:24:32Z

@nizarsalhaji94 hi sorry I am no longer involved with this project since February 2023. Try asking @eddyv

nizarsalhaji94 · 2023-12-21T11:46:30Z

Hi @eddyv

Is a released date is planned ?
I’m interested to use SHUTDOWN Terminal Failure Reatction.
In fact, we need to shutdown (or stop or kill) the PC after max retry.

Thanks

rkolesnev · 2024-03-01T17:14:28Z

@nizarsalhaji94
We are planning to implement this feature at some point, but no concrete dates / roadmap for this yet.
As a work around - i have suggested to do the stop/kill from user function approach - in this issue #718 (comment)

Just in general - i haven't seen your comment here until now - in the future - it would be more visible if you raise it as a question / issue with link to the PR if its relevant.

START: Explicit retry exception for cleaner logging

1a18281

astubbs linked an issue May 6, 2022 that may be closed by this pull request

Let user function throw retry exception, instead of raw errors which pollute log #242

Closed

2 tasks

switch tests to utilise

6af5beb

ifeeneyconfluent reviewed May 12, 2022

View reviewed changes

...src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java Outdated Show resolved Hide resolved

...src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java Outdated Show resolved Hide resolved

ifeeneyconfluent and others added 5 commits May 12, 2022 14:00

Merge branch 'master' into features/retry-exception

3d704c3

alternative handling flow

7f1cd22

use Slf4j alpha 7 for dynamic log level - check what else is new in 2

a3331e5

Merge remote-tracking branch 'antony/features/retry-exception' into f…

33454ad

…eatures/retry-exception

wording

4d2eaa1

astubbs requested a review from rkolesnev May 12, 2022 14:57

astubbs added 4 commits May 13, 2022 12:03

step

28dd391

step

1c37ebf

Merge remote-tracking branch 'confluent/master' into features/retry-e…

8a62163

…xception

step

5b2b515

ifeeneyconfluent approved these changes May 13, 2022

View reviewed changes

astubbs added 2 commits May 13, 2022 13:35

terminal exception and handling

a9ded2d

handle terminal

1a72f3c

astubbs mentioned this pull request May 13, 2022

feature: #242 Explicit retriable exception for cleaner ERROR logging #268

Closed

3 tasks

astubbs changed the title ~~feature: #242 Explicit retry exception for cleaner logging~~ feature: #242 Explicit terminal and retriable exceptions for cleaner logging and poison pills May 13, 2022

astubbs mentioned this pull request May 13, 2022

NoSuchFieldException when using consumer inherited from KafkaConsumer #195

Closed

rkolesnev requested changes May 13, 2022

View reviewed changes

astubbs added 6 commits May 13, 2022 16:42

introduce Offsets class

fb862d6

step

1f662d9

step

830717a

test and batch drafting

e778a09

test and batch drafting

7d56bad

test and batch drafting

eb42645

astubbs marked this pull request as draft May 14, 2022 08:01

astubbs requested a review from ifeeneyconfluent May 14, 2022 08:01

astubbs requested a review from rkolesnev May 14, 2022 08:01

test and batch drafting

8a8a780

astubbs changed the title ~~feature: #242 Explicit terminal and retriable exceptions for cleaner logging and poison pills~~ feature: #242 Explicit terminal and retry exceptions for cleaner logging and poison pills May 16, 2022

astubbs mentioned this pull request May 19, 2022

Handle deserialization exceptions thrown from the deserialiser #304

Open

astubbs added 2 commits May 20, 2022 11:37

Merge remote-tracking branch 'origin/master' into features/retry-exce…

486caa5

…ption # Conflicts: # parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java # parallel-consumer-core/src/test/resources/logback-test.xml

revert partial batch failure

93324d0

astubbs commented May 20, 2022

View reviewed changes

review

14aad84

rkolesnev reviewed May 20, 2022

View reviewed changes

astubbs mentioned this pull request Jul 15, 2022

major: Extend Consumer and Function for more cohesive API #303

Closed

2 tasks

astubbs mentioned this pull request Jul 27, 2022

Dead letter queue (DLQ) as option for retry expiration #366

Closed

2 tasks

astubbs removed a link to an issue Oct 21, 2022

Let user function throw retry exception, instead of raw errors which pollute log #242

Closed

2 tasks

eddyv closed this Jun 15, 2023

rkolesnev mentioned this pull request Mar 5, 2024

Missing feature to terminate processing #718

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: #242 Explicit terminal and retry exceptions for cleaner logging and poison pills #291

feature: #242 Explicit terminal and retry exceptions for cleaner logging and poison pills #291

astubbs commented May 6, 2022 •

edited

Loading

astubbs commented May 12, 2022

ifeeneyconfluent left a comment

rkolesnev May 13, 2022

rkolesnev May 13, 2022

astubbs May 14, 2022

rkolesnev May 13, 2022

astubbs May 14, 2022

astubbs commented May 17, 2022

astubbs left a comment

astubbs May 20, 2022

astubbs May 20, 2022

astubbs May 20, 2022

rkolesnev May 16, 2022

rkolesnev May 16, 2022

rkolesnev May 16, 2022

astubbs Jul 12, 2022

rkolesnev May 16, 2022

eddyv commented Jun 15, 2023

nizarsalhaji94 commented Dec 20, 2023

astubbs commented Dec 21, 2023

nizarsalhaji94 commented Dec 21, 2023

rkolesnev commented Mar 1, 2024

	log.debug("Explicit " + RetriableException.class.getSimpleName() + " caught, logging at DEBUG only. " + msg, e);
	log.debug("Explicit {} caught - {}", RetriableException.class.getSimpleName(), msg, e);

+ */
+/**
+ * todo

feature: #242 Explicit terminal and retry exceptions for cleaner logging and poison pills #291

feature: #242 Explicit terminal and retry exceptions for cleaner logging and poison pills #291

Conversation

astubbs commented May 6, 2022 • edited Loading

astubbs commented May 12, 2022

ifeeneyconfluent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astubbs commented May 17, 2022

astubbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyv commented Jun 15, 2023

nizarsalhaji94 commented Dec 20, 2023

astubbs commented Dec 21, 2023

nizarsalhaji94 commented Dec 21, 2023

rkolesnev commented Mar 1, 2024

astubbs commented May 6, 2022 •

edited

Loading