Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CommonDelegatingErrorHandler: How to traverse exceptions in order to find a correct delegate #2288

Closed
garyrussell opened this issue May 31, 2022 Discussed in #2283 · 3 comments
Labels
backport 2.8.x (obsolete) backport 2.9.x (obsolete) ideal-for-user-contribution An issue that would ideal for a user to get started with contributing.
Milestone

Comments

@garyrussell
Copy link
Contributor

Add an option to traverse the cause chain to find a delegate.

Discussed in #2283

Originally posted by amseager May 25, 2022
Currently, everything in this handler uses this private method:

@Nullable
	private CommonErrorHandler findDelegate(Throwable thrownException) {
		Throwable cause = thrownException;
		if (cause instanceof ListenerExecutionFailedException) {
			cause = thrownException.getCause();
		}
		if (cause != null) {
			Class<? extends Throwable> causeClass = cause.getClass();
			for (Entry<Class<? extends Throwable>, CommonErrorHandler> entry : this.delegates.entrySet()) {
				if (entry.getKey().isAssignableFrom(causeClass)) {
					return entry.getValue();
				}
			}
		}
		return null;
	}

As you can see, if we have ListenerExecutionFailedException, we'll get its cause and find an appropriate delegate.

However, this wouldn't work in a lot of cases when we have our set of exceptions wrapped in something else, e.g. in MessageConversionException (I think it's the most popular case). And we cannot use MessageConversionException as a delegates map key since we want to have a different logic for each underlying exception (let's say we want custom DefaultErrorHandler for ConstraintViolationException with a fixed backoff and its own recoverer and the another one for any other exception with ExponentialBackOff without a recoverer at all - but both are wrapped in the same MessageConversionException).

How to solve this problem? I don't want to write a custom DelegatingErrorHandler because I'll need to copy almost all the code from the CommonDelegatingErrorHandler.
Previously, we had ConditionalDelegatingErrorHandler which had handle method being public so it could've been ok to override it but now it's deprecated in favor of CommonDelegatingErrorHandler.

@garyrussell garyrussell added ideal-for-user-contribution An issue that would ideal for a user to get started with contributing. backport 2.8.x (obsolete) backport 2.9.x (obsolete) labels May 31, 2022
@garyrussell garyrussell added this to the 3.0 Backlog milestone May 31, 2022
@breader124
Copy link
Contributor

Hi @garyrussell, could you assign it to me?

@artembilan
Copy link
Member

Hi, @breader124 !

It’s fine to take the issue just saying that you’re up. We can assign issues only to team members .

thank you for considering to contribute!

breader124 added a commit to breader124/spring-kafka that referenced this issue Jun 19, 2022
Fixes spring-projects#2288

Add possibility to deeply traverse cause chain in order to find proper delegate
for handling thrown exception. Keep old way of cause chain traversing as default
one.

Cover new code with unit test.
@breader124
Copy link
Contributor

PR created, please give it a look: #2310

breader124 added a commit to breader124/spring-kafka that referenced this issue Jun 19, 2022
Use BinaryExceptionClassifier while traversing cause chain to make it possible
to classify throwables for handling based on inheritance etc.
breader124 added a commit to breader124/spring-kafka that referenced this issue Jun 25, 2022
garyrussell pushed a commit that referenced this issue Jul 7, 2022
Resolves #2288

GH-2288: Add option to deeply traverse exc chain

Add possibility to deeply traverse cause chain in order to find proper delegate
for handling thrown exception. Keep old way of cause chain traversing as default
one.

Cover new code with unit test.

GH-2288: Make cause traversing more extensible

Use BinaryExceptionClassifier while traversing cause chain to make it possible
to classify throwables for handling based on inheritance etc.

GH-2288: Remove custom BinaryExceptionClassifier

Polish Javadocs.
garyrussell pushed a commit that referenced this issue Jul 7, 2022
Resolves #2288

GH-2288: Add option to deeply traverse exc chain

Add possibility to deeply traverse cause chain in order to find proper delegate
for handling thrown exception. Keep old way of cause chain traversing as default
one.

Cover new code with unit test.

GH-2288: Make cause traversing more extensible

Use BinaryExceptionClassifier while traversing cause chain to make it possible
to classify throwables for handling based on inheritance etc.

GH-2288: Remove custom BinaryExceptionClassifier

Polish Javadocs.
@garyrussell garyrussell modified the milestones: 3.0 Backlog, 3.0.0-M5 Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.8.x (obsolete) backport 2.9.x (obsolete) ideal-for-user-contribution An issue that would ideal for a user to get started with contributing.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants