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

Support notifying bank created slot status in geyser #3126

Merged

Conversation

lijunwangs
Copy link

@lijunwangs lijunwangs commented Oct 9, 2024

Problem

This is the 3rd part to support more slot status notification type in geyser. CreatedBank on new fork creation.

Summary of Changes

Link SlotStatusNotifier in replay stage on CreatedBank.

Tests: tested change with Postgres geyser plugin and confirming the CreatedBank being received.

Fixes #

#2957

@lijunwangs lijunwangs force-pushed the support_notify_bank_created_in_geyser branch from c195658 to 2048633 Compare October 9, 2024 23:48
@lijunwangs lijunwangs marked this pull request as draft October 10, 2024 01:03
@lijunwangs lijunwangs marked this pull request as ready for review October 10, 2024 23:23
Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

It would be good to add a unit test that covers the path of actually sending/receiving this notification, but the code LGTM

@@ -9023,6 +9047,7 @@ pub(crate) mod tests {
&poh_recorder,
&leader_schedule_cache,
&rpc_subscriptions,
&None,

Choose a reason for hiding this comment

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

Can we add/modify a test that provides a SlotStatusNotifier to get some coverage?

Copy link
Author

Choose a reason for hiding this comment

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

Will handle it in another PR.

@lijunwangs lijunwangs merged commit 5bcf161 into anza-xyz:master Oct 15, 2024
38 checks passed
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.

2 participants