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 hanging critical error #996

Merged
merged 3 commits into from Jun 20, 2023
Merged

Conversation

ghubertpalo
Copy link
Collaborator

@ghubertpalo ghubertpalo commented Jun 20, 2023

Content

This PR includes...

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

None

Issue(s)

Closes #993

@ghubertpalo ghubertpalo temporarily deployed to testing-preview June 20, 2023 08:01 — with GitHub Actions Inactive
@ghubertpalo ghubertpalo force-pushed the greg/993/aggregator_critical_error branch from 7533f3a to 1a8c8fc Compare June 20, 2023 09:44
@ghubertpalo ghubertpalo temporarily deployed to testing-preview June 20, 2023 09:52 — with GitHub Actions Inactive
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Just a question about an unwrap

@@ -244,7 +244,7 @@ impl ServeCommand {
let _ = shutdown_tx.send(());

info!("Event store is finishing...");
event_store_thread.await?;
event_store_thread.await.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any specific reason why you unwrapped here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it should not change anything since the event store returns Ok(()) I wanted to make it to fail as fast as possible in case of … whatever.

@ghubertpalo ghubertpalo merged commit da825b4 into main Jun 20, 2023
23 checks passed
@ghubertpalo ghubertpalo deleted the greg/993/aggregator_critical_error branch June 20, 2023 11:29
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.

Aggregator does not exit on critical error
2 participants