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

Support for setting uncaught exception handler #780

Merged
merged 10 commits into from
Aug 28, 2023

Conversation

guillermocalvo
Copy link
Contributor

Closes #424

@guillermocalvo guillermocalvo added the type: enhancement New feature or request label Jul 18, 2023
@guillermocalvo guillermocalvo linked an issue Jul 18, 2023 that may be closed by this pull request
@guillermocalvo guillermocalvo force-pushed the 424-support-for-setting-uncaught-exception-handler branch from 412e917 to de8b0f3 Compare July 19, 2023 15:32
@guillermocalvo guillermocalvo force-pushed the 424-support-for-setting-uncaught-exception-handler branch from de8b0f3 to 9eb8fb3 Compare July 21, 2023 08:33
@guillermocalvo
Copy link
Contributor Author

SonarCloud Quality Gate failed.    Quality Gate failed

33.3% 33.3% Coverage

@sdelamo

SonarQube keeps reporting low coverage, but I added specific tests to cover new code.

When I run them locally, I get 100% coverage so I'm wondering if there might be something wrong in the project config.

I tried several approaches to try and make SonarQube acknowledge the actual code coverage:

  • I made KafkaStreamsFactory.setUncaughtExceptionHandler package-protected instead of private.
  • I made KafkaStreamsFactory.setUncaughtExceptionHandler instance method instead of static.
  • I made KafkaStreamsFactorySpec start an actual kafka container instead of simply invoking the method.

But sadly, no luck.

My next approach would be to move the new code to its own @Singleton and make KafkaStreamsFactory depend on it, but I think it may be a bit overkill.

Any ideas what I can do to convince SonarQube that the new code is actually covered by the new tests?

LOG.warn("Ignoring illegal exception handler: {}. Please use one of: {}", action,
asList(StreamThreadExceptionResponse.values()));
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do something like?

    /**
     * Make an uncaught exception handler for a given kafka streams configuration.
     *
     * @param properties The kafka streams configuration.
     * @return An optional exception handler if {@code uncaught-exception-handler} was configured.
     */
    private Optional<StreamsUncaughtExceptionHandler> makeUncaughtExceptionHandler(Properties properties) {
        return Optional.ofNullable(properties.getProperty(UNCAUGHT_EXCEPTION_HANDLER_PROPERTY))
            .filter(not(String::isBlank))
            .flatMap(this::makeUncaughtExceptionHandler);
    }
    
    private Optional<StreamsUncaughtExceptionHandler> makeUncaughtExceptionHandler(String action) {
                try {
                    final StreamThreadExceptionResponse response = StreamThreadExceptionResponse.valueOf(action.toUpperCase());
                    return exception -> {
                        if (LOG.isWarnEnabled()) {
                            LOG.warn("Responding with {} to unexpected exception thrown by kafka stream thread", response, exception);
                        }
                        return Optional.of(response);
                    };
                } catch (IllegalArgumentException e) {
                    if (LOG.isWarnEnabled()) {
                        LOG.warn("Ignoring illegal exception handler: {}. Please use one of: {}", action,
                            asList(StreamThreadExceptionResponse.values()));
                    }
                    return Optional.empty();
                }
            });
    }

@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

88.9% 88.9% Coverage
0.0% 0.0% Duplication

@sdelamo sdelamo merged commit e75ed8e into master Aug 28, 2023
8 checks passed
@sdelamo sdelamo deleted the 424-support-for-setting-uncaught-exception-handler branch August 28, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support for setting uncaught exception handler
3 participants