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

geyser: add flag geyser-plugin-snapshot-disabled #4103

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

fanatid
Copy link

@fanatid fanatid commented Dec 13, 2024

Problem

Startup time with an enabled geyser interface is significant and CPU load does not exceed 100%. Disable snapshot on nodes that need geyser but do not need snapshot reduce startup time on 15-20min.

Summary of Changes

PR introduces a new flag --geyser-plugin-snapshot-disabled, if flag is set then account notifications from snapshot to geyser are ignored and the node starts faster while geyser interface is still enabled.

@fanatid fanatid force-pushed the geyser-snapshot-disable-flag branch from 194a7d3 to d48cdb9 Compare December 13, 2024 01:56
@fanatid fanatid force-pushed the geyser-snapshot-disable-flag branch from d48cdb9 to 4c23162 Compare December 13, 2024 02:08
@fanatid fanatid force-pushed the geyser-snapshot-disable-flag branch from 4c23162 to 526eef9 Compare December 13, 2024 02:13

slots.sort_by(|a, b| b.cmp(a));
for slot in slots {
self.notify_accounts_in_slot(slot, &mut notified_accounts, &mut notify_stats);

Choose a reason for hiding this comment

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

Do you know why this code take minutes?

Choose a reason for hiding this comment

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

because of amount of data -- could be 100 millions of records

@steviez
Copy link

steviez commented Dec 16, 2024

Granted nobody ever reviewed this PR, but this is looking to accomplish the same thing that #2022 does right ?

validator/src/cli.rs Outdated Show resolved Hide resolved
@fanatid
Copy link
Author

fanatid commented Dec 16, 2024

@steviez oh wow, I remember that PR and my PR doing the same just in different way! Actually, I think that #2022 way to define snapshot requirement through an additional method looks much better than the flag.

@steviez @lijunwangs what do you think, if:

  • we remove CLI flag --geyser-plugin-snapshot-disabled from this PR
  • add function account_data_snapshot_notifications_enabled (should we add default value? true for backward compatibility?)
  • if any plugin return true from new function then we stream snapshot data, otherwise skip (as with flag in this PR)

@fanatid fanatid force-pushed the geyser-snapshot-disable-flag branch 2 times, most recently from 3d5b2c5 to 7acf15c Compare December 16, 2024 22:01
@fanatid fanatid force-pushed the geyser-snapshot-disable-flag branch from 7acf15c to b7801ee Compare December 16, 2024 22:13
@fanatid
Copy link
Author

fanatid commented Dec 16, 2024

Updated from CLI argument to Geyser interface function

@lijunwangs
Copy link

@steviez oh wow, I remember that PR and my PR doing the same just in different way! Actually, I think that #2022 way to define snapshot requirement through an additional method looks much better than the flag.

@steviez @lijunwangs what do you think, if:

  • we remove CLI flag --geyser-plugin-snapshot-disabled from this PR
  • add function account_data_snapshot_notifications_enabled (should we add default value? true for backward compatibility?)
  • if any plugin return true from new function then we stream snapshot data, otherwise skip (as with flag in this PR)

That sounds okay -- so we allow the plugin to opt in.

@lijunwangs lijunwangs merged commit 22c8951 into anza-xyz:master Dec 19, 2024
40 checks passed
@fanatid fanatid deleted the geyser-snapshot-disable-flag branch December 19, 2024 04:43
@fanatid
Copy link
Author

fanatid commented Dec 19, 2024

@lijunwangs thank you! is it possible to create a backport to v2.1?

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.

4 participants