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 dead slot notification in geyser #3163

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lijunwangs
Copy link

@lijunwangs lijunwangs commented Oct 14, 2024

Problem

This is the 4th part to support more slot status notification type in geyser: Dead slot.

Summary of Changes

Link SlotStatusNotifier in replay stage on Dead slot.

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

Fixes #

#2957

@lijunwangs lijunwangs force-pushed the support_dead_slot_notification_in_geyser branch from 8b09500 to a6cf0e1 Compare October 15, 2024 06:25
@lijunwangs lijunwangs force-pushed the support_dead_slot_notification_in_geyser branch from a6cf0e1 to db5ad2b Compare October 21, 2024 01:20
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.

Looks good to me. Left 1 consideration

@@ -328,6 +328,9 @@ pub enum SlotStatus {

/// A new bank fork is created with the slot
CreatedBank,

/// A slot is marked dead
Dead,

Choose a reason for hiding this comment

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

Should we include the reason for marking the slot dead as a string in this enum?

@@ -37,6 +37,10 @@ impl SlotStatusNotifierInterface for SlotStatusNotifierImpl {
fn notify_created_bank(&self, slot: Slot, parent: Slot) {
self.notify_slot_status(slot, Some(parent), SlotStatus::CreatedBank);
}

fn notify_slot_dead(&self, slot: Slot, _error: String) {

Choose a reason for hiding this comment

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

It would be nice to include this error string.. maybe embed it in the SlotStatus enum

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