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

refactor: Strict/Non-strict mode improvements #164

Merged
merged 7 commits into from
Sep 9, 2021

Conversation

khorolets
Copy link
Member

@khorolets khorolets commented Sep 7, 2021

Fixes #163

I've bumped the version to 0.10.0 because I've dropped the --allow-missing-relations-in-first-blocks flag completely. It doesn't mean this PR is going to be released once merged.

  • Dropped --allow-missing-relations-in-first-blocks flag completely
  • Add the flag --non-strict-mode which:
    • changes retry attempts on Receipts from 10 to 4
    • stops saving account_changes completely as it is impossible to save in case of missing of related Receipts
  • Add the flag --stop-after-number-of-blocks N which namely stops the indexer after processing N blocks

All those flags and features are useful for debugging or emergency purposes. They are not supposed to be used in normal circumstances.

@khorolets khorolets requested a review from frol September 7, 2021 13:12
CHANGELOG.md Outdated Show resolved Hide resolved
src/configs.rs Outdated Show resolved Hide resolved
src/configs.rs Outdated Show resolved Hide resolved
src/configs.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
"Stopping after according to `--stop-after-number-of-blocks {}`",
index_to_stop,
);
std::process::exit(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

CRITICAL: This might corrupt the database. We should shut nearcore down gracefully (ask @miraclx if you need help)

src/main.rs Outdated
Comment on lines 134 to 135
if let Some(index_to_stop) = stop_after_number_of_blocks {
if (index_to_stop) as usize == index {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use .take(stop_after_number_of_blocks) on the iterator? This will gracefully handle all the messages in the while loop below and just exit from Indexing (we will only need to gracefully shut down nearcore after we complete here).

Copy link
Contributor

@frol frol left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me!

use clap::Clap;
use std::convert::{TryFrom, TryInto};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? std imports are more generic, so should be above

src/main.rs Outdated
});
let mut handle_messages = if let Some(stop_after_n_blocks) = stop_after_number_of_blocks {
handle_messages
.take(stop_after_n_blocks as usize)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would either make stop_after_number_of_blocks to be usize in the first place, or use usize::try_from().expect() since as usize might cause unexpected issues on non-64-bit platforms down the road.

src/configs.rs Outdated
pub non_strict_mode: bool,
/// Stops indexer completely after indexing the provided number of blocks
#[clap(long, short)]
pub stop_after_number_of_blocks: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be NonZero?

src/main.rs Outdated

while let Some(_handled_message) = handle_messages.next().await {}
// Graceful shutdown
drop(handle_messages);
tokio::time::sleep(std::time::Duration::from_secs(5)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a log here that the graceful shutdown will happen in 5 seconds, so whoever uses it, don't think it got stuck.

src/main.rs Outdated
join!(
execution_outcomes_future,
accounts_future,
access_keys_future,
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing access keys may also fail in non-strict mode, so we should skip them.

P.S. Update the Clap help to indicate this.

@khorolets khorolets merged commit 6d896fa into master Sep 9, 2021
@khorolets khorolets deleted the refactor/fix-allow-missing-relations branch September 9, 2021 12:01
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.

Fix --allow-missing-relations-in-first-blocks
2 participants