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

Fix null redisClient during validation, add more logs #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

s01cy
Copy link

@s01cy s01cy commented Mar 7, 2025

I've noticed a bug in RedisExporter - it is trying to do this:

During exporter validation, Zeebe tries to test the lifecycle methods of the exporter
The validation process was calling close() without first calling open()
This led to redisClient being null when close() was called

@CLAassistant
Copy link

CLAassistant commented Mar 7, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@VonDerBeck
Copy link
Collaborator

@s01cy
Thanks a lot for this! Even small changes making the exporter more stable are more than welcome. 👍

Can you do me a favour and fix the failing RedisLateStartupTest? It fails due to your changed log output - the test uses log messages as an indicator e.g. in order to detect when the initial connection to Redis has failed etc.

Gunnar

@VonDerBeck VonDerBeck self-requested a review March 7, 2025 13:53
@VonDerBeck VonDerBeck self-assigned this Mar 7, 2025
} catch (RedisConnectionException ex) {
if (!fullyLoggedStartupException) {
logger.error("Failure connecting Redis exporter to " + config.getRemoteAddress().get(), ex);
logger.error("Redis connection error: Failed to connect to {}. Error details: {}",
config.getRemoteAddress().get(), ex.getMessage(), ex);
Copy link
Collaborator

@VonDerBeck VonDerBeck Mar 7, 2025

Choose a reason for hiding this comment

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

This change is ok. But needs to be considered in RedisLateStartupTest line 72/73 where the test waits until the exporter failed to connect to Redis. This ensures that the later startup of Redis orchestrated by the test will be really after this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants