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(error): Bubble error up to main when connecting to Redis fails #130

Merged
merged 6 commits into from
Oct 18, 2022

Conversation

alexsnaps
Copy link
Member

No magic here…

Fixes #100

@alexsnaps alexsnaps self-assigned this Oct 11, 2022
@alexsnaps alexsnaps requested a review from eguzki October 11, 2022 08:14
Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

What about the redis storage sync version? If the connection cannot be established, the error is panicked. That is what we wanted to avoid, right?

@alexsnaps
Copy link
Member Author

What about the redis storage sync version? If the connection cannot be established, the error is panicked. That is what we wanted to avoid, right?

Yeah, I mostly cared about the server binary, which leverages the async version. But yeah, I should totally do the sync version as well… otherwise this is weird.

Both in sync redis as well as the cached version
@alexsnaps alexsnaps force-pushed the issue_100 branch 2 times, most recently from a12935f to ba5134d Compare October 17, 2022 14:46
// this panic!s And I really don't see how to bubble the redis error back up:
// r2d2 consumes it
// RedisError are not publicly constructable
// So using String as error type… sad
Copy link
Member Author

Choose a reason for hiding this comment

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

See #131

.unwrap();
) -> Result<Self, RedisError> {
let info = ConnectionInfo::from_str(redis_url)?;
let redis_conn_manager = ConnectionManager::new(redis::Client::open(info).unwrap()).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: maybe expect instead of unwrap for consistency?

@alexsnaps alexsnaps merged commit d5cb6f1 into main Oct 18, 2022
@alexsnaps alexsnaps deleted the issue_100 branch October 18, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate complete environment on startup
2 participants