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

[3.2] log integrity hash on start/stop with integrity-hash-on-start & integrity-hash-on-stop options #395

Merged
merged 4 commits into from
Jun 21, 2022

Conversation

spoonincode
Copy link
Member

Unequivocally logging the integrity hash after loading a snapshot increases start up time from a snapshot by nearly 33%. Most users probably don't even notice the log; let's not always do that.

Log integrity hash on start up and shut down via new integrity-hash-on-start & integrity-hash-on-stop options.

@@ -723,6 +724,9 @@ struct controller_impl {
~controller_impl() {
thread_pool.stop();
pending.reset();
//using the presence of a row in datebase_header index to indicate controller had a successful startup()
Copy link
Member Author

Choose a reason for hiding this comment

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

eh something better?

Copy link
Member

Choose a reason for hiding this comment

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

Move the chain_plugin where you know if started successfully?

Copy link
Member Author

@spoonincode spoonincode Jun 15, 2022

Choose a reason for hiding this comment

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

I can't log the startup in chain_plugin because I need to to do the log before replay but after snapshot is loaded. So, it felt weird to do the startup log in controller but the shutdown log in chain_plugin.

I'm not sure doing it in chain_plugin really helps either since there are so many exceptions that can start unrolling the world at any time.

Actually I'm not really even sure this here works well enough: what if an exception is thrown half way through loading a snapshot. Then once everything unrolls the dtor is fired and if datebase_header made it then it does an integrity computation with a half initialized state? That's what I want to avoid.

But I also wanted to avoid adding a bool yep_controller_started_up. It's kinda wild there isn't a clear indicator anywhere 🤔

Copy link
Member

Choose a reason for hiding this comment

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

In chain_plugin::plugin_startup you could add a catch(...) after the database_guard_exception catch and add a my->chain.reset();. Then in chain_plugin::plugin_shutdown() you could check my->chain to know if it is valid, if so then log the integrity.

Copy link
Member Author

Choose a reason for hiding this comment

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

I capitulated and just added a bool 😕

@matthewdarwin
Copy link

Also for 3.1?

@spoonincode spoonincode changed the title RFC: log integrity hash on start/stop with integrity-hash-on-start & integrity-hash-on-stop options log integrity hash on start/stop with integrity-hash-on-start & integrity-hash-on-stop options Jun 17, 2022
@spoonincode spoonincode marked this pull request as ready for review June 17, 2022 19:28
@spoonincode
Copy link
Member Author

Also for 3.1?

Certainly a bit of a stretch by the typical playbook but maybe we can have that discussion since it's such a few line difference.

@spoonincode spoonincode merged commit 91bf1f1 into main Jun 21, 2022
@spoonincode spoonincode deleted the log_integrity_hash_startstop branch June 21, 2022 16:41
@arhag arhag changed the title log integrity hash on start/stop with integrity-hash-on-start & integrity-hash-on-stop options [3.2] log integrity hash on start/stop with integrity-hash-on-start & integrity-hash-on-stop options Jul 1, 2022
@arhag arhag added this to the Mandel 3.2.0-rc1 milestone Jul 1, 2022
@arhag arhag modified the milestones: Mandel 3.2.0-rc1, Mandel 3.2.0 Aug 17, 2022
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.

4 participants