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

Cleanup streaming errors for block not ready #5632

Conversation

AndriiDiachuk
Copy link
Contributor

@AndriiDiachuk AndriiDiachuk commented Apr 5, 2024

Closes: #5574

@AndriiDiachuk AndriiDiachuk marked this pull request as ready for review April 5, 2024 14:15
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.45%. Comparing base (ad12394) to head (b1471c5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5632      +/-   ##
==========================================
- Coverage   55.55%   55.45%   -0.11%     
==========================================
  Files        1043      991      -52     
  Lines      102034    97588    -4446     
==========================================
- Hits        56689    54113    -2576     
+ Misses      40992    39250    -1742     
+ Partials     4353     4225     -128     
Flag Coverage Δ
unittests 55.45% <100.00%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Guitarheroua Guitarheroua self-requested a review April 9, 2024 07:53
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

good start. I think there are a few more places to update as well.

we're looking to simplify this list in the streamer:

if errors.Is(err, storage.ErrNotFound) ||
errors.Is(err, storage.ErrHeightNotIndexed) ||
execution_data.IsBlobNotFoundError(err) ||
errors.Is(err, ErrBlockNotReady) {
// no more available
return nil
}

to just be

if errors.Is(err, ErrBlockNotReady) {
	return nil
}

So all code that returns errors to subscription.Next will need to have updated error handling that converts one those errors into subscription.ErrBlockNotReady. Other errors can be returned normally so they get handled as they do today.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

please also update the other streaming endpoints in a similar way

engine/access/state_stream/backend/backend.go Outdated Show resolved Hide resolved
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

please also update

if errors.Is(err, storage.ErrNotFound) ||
errors.Is(err, storage.ErrHeightNotIndexed) ||
execution_data.IsBlobNotFoundError(err) ||
errors.Is(err, ErrBlockNotReady) {
// no more available
return nil
}

to only match on subscription.ErrBlockNotReady

engine/access/rpc/backend/backend_stream_blocks.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_stream_blocks.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/backend.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

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

I added a couple of small comments. Otherwise, it looks good!

@peterargue peterargue enabled auto-merge April 23, 2024 15:44
@peterargue peterargue added this pull request to the merge queue Apr 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 24, 2024
@franklywatson franklywatson added this pull request to the merge queue Apr 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 24, 2024
@franklywatson franklywatson added this pull request to the merge queue Apr 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 24, 2024
@peterargue peterargue added this pull request to the merge queue Apr 24, 2024
@peterargue peterargue removed this pull request from the merge queue due to a manual request Apr 24, 2024
@peterargue peterargue added this pull request to the merge queue Apr 25, 2024
Merged via the queue into onflow:master with commit 4246f1a Apr 25, 2024
55 checks passed
@AndriiDiachuk AndriiDiachuk deleted the cleanup-streaming-errors-for-block-not-ready branch May 10, 2024 11:27
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.

[Access] Cleanup streaming errors for block not ready
5 participants