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

Add geyser block-metadata notification with entry count #33359

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

Geyser stream consumers don't know how many entries to expect.

Summary of Changes

Include entry count in block-metadata notification

Fixes #32988

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #33359 (8a62aaa) into master (3e8ccbe) will increase coverage by 0.0%.
Report is 1 commits behind head on master.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #33359   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         796      796           
  Lines      215842   215853   +11     
=======================================
+ Hits       176910   176921   +11     
  Misses      38932    38932           

Copy link
Contributor

@lijunwangs lijunwangs 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 on geyser side change. I will defer to @carllin on the entry_count is real entry_count in the block.

@CriesofCarrots
Copy link
Contributor Author

I will defer to @carllin on the entry_count is real entry_count in the block.

Exactly the bit I was hoping @carllin would review :) Carl, please sanity check; I think we concluded that this would be a legit way to use progress_map.

@carllin
Copy link
Contributor

carllin commented Sep 26, 2023

Looks right! Confirming this is necessary in addition to the existing entry notifications here (which includes the entry index)?

if let Some(entry_notification_sender) = entry_notification_sender {
let entry_index = progress.num_entries.saturating_add(i);
if let Err(err) = entry_notification_sender.send(EntryNotification {
slot,
index: entry_index,
entry: entry.into(),
}) {
warn!(
"Slot {}, entry {} entry_notification_sender send failed: {:?}",
slot, entry_index, err
);
}
. The client can't conclude when the block is over by the latest entry notification?

@CriesofCarrots
Copy link
Contributor Author

The client can't conclude when the block is over by the latest entry notification?

Correct, there's no way for a client to know which entry notification is the expected last one. Technically, there currently exists a way to determine this by tracking all transactions and using the executed_transaction_count to find the last transaction and look at its entry index, but this forces clients to follow all transactions, even if they are only interested in entry notifications.

@CriesofCarrots CriesofCarrots merged commit ddd0297 into solana-labs:master Sep 26, 2023
15 checks passed
vovkman pushed a commit to helius-labs/solana that referenced this pull request Oct 19, 2023
…33359)

* Add new ReplicaBlockInfoVersions variant

* Use new variant to return entry count
godmodegalactus pushed a commit to godmodegalactus/solana that referenced this pull request Dec 28, 2023
…33359)

* Add new ReplicaBlockInfoVersions variant

* Use new variant to return entry count
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.

geyser: add entries_count field to block metadata
3 participants