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

Add Flag to Reset the DB on Shutdown. #202

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

favalos
Copy link
Contributor

@favalos favalos commented Jan 10, 2024

Pull request to add the flag discussed in #97.

I had to add a reset flag to every DB configuration and change the Reset implementation.

Other approach could be doing the implementation on the Shutdown method, but that would imply to add a new Resetable() bool method to the subsystem interface.

Please let me know your comments.

@dfarr
Copy link
Member

dfarr commented Jan 10, 2024

Thank you so much for the contribution @favalos!

I have one (hopefully) small change to ask.

The shutdown function on the api/aio interfaces are used as a signal to begin a graceful shutdown. The aio method is currently unused, but we do use it on the api side. When this signal is received we stop taking new requests but continue to process any sqes/cqes already in the system until all are complete. As such we can't reset the database via the shutdown function as we still need to write to the database after this function has been called.

You can see the shutdown process here.

  1. shutdown signal received (happens in a go routine and calls api.shutdown+aio.shutdown via system.Shutdown)
  2. system.Loop will return once all outstanding sqes/cqes are complete
  3. api.Stop() called
  4. aio.Stop() called (internally calls Stop for all subsystems)

I recommend moving the call to Reset (if the flag is true) the Stop function, this is when we can safely reset.

@favalos
Copy link
Contributor Author

favalos commented Jan 11, 2024

Hi @dfarr,

Thanks for the detailed explanation, I did the changes accordingly.

Let me know if that looks fine.

@guergabo guergabo requested a review from dfarr January 11, 2024 03:32
@guergabo guergabo added the enhancement New feature or request label Jan 11, 2024
@guergabo
Copy link
Contributor

Thank you @favalos for contributing to resonate !!!

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (d4fb16c) 64.85% compared to head (ca6cdcd) 64.81%.

Files Patch % Lines
...rnal/app/subsystems/aio/store/postgres/postgres.go 33.33% 3 Missing and 1 partial ⚠️
internal/app/subsystems/aio/store/sqlite/sqlite.go 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
- Coverage   64.85%   64.81%   -0.05%     
==========================================
  Files          83       83              
  Lines        8829     8841      +12     
==========================================
+ Hits         5726     5730       +4     
- Misses       2709     2715       +6     
- Partials      394      396       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dfarr dfarr left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you @favalos

@dfarr
Copy link
Member

dfarr commented Jan 11, 2024

Closes #97

@dfarr dfarr merged commit ae80993 into resonatehq:main Jan 11, 2024
5 of 7 checks passed
@favalos favalos deleted the fresh-db-#97 branch January 12, 2024 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants