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

feat(failover): return 503 to batcher when eigenda is down #193

Merged
merged 11 commits into from
Nov 11, 2024

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Oct 25, 2024

Depends on Layr-Labs/eigenda#828

Would appreciate a quick review for the logic, but leaving as draft because I think I will refactor the api errors that we return from eigenda-client (don't like api.ErrorAPI name and also using http status codes in them).

TODO:

  • populate ConfirmationTimeout which was added in eigenda-client: c479458
  • merge fix(e2e tests): missing new eigenda-client required config fields #196 first and then rebase on top to fix the ci test failing: 727a63d
  • add tests to test failover behavior
    • unit: 8a8fcd6
    • integration: very very hard to do with the current setup we have. Feel like we should first integration-test the failover behavior in disperser-client, and then once we've figured that one out then we could refactor the testing framework in proxy to extend that one. Created a tech-debt project in Linear and added the integration-test for disperser-client as a first issue in there.

Fixes Issue

Fixes #

Changes proposed

Screenshots (Optional)

Note to reviewers

@samlaf samlaf requested a review from bxue-l2 October 25, 2024 16:24
@samlaf samlaf marked this pull request as draft October 25, 2024 16:24
go.mod Outdated Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
store/generated_key/eigenda/eigenda.go Show resolved Hide resolved
store/generated_key/eigenda/eigenda.go Outdated Show resolved Hide resolved
@samlaf samlaf force-pushed the samlaf/feat--503-error-logic-with-retries branch from 4b77864 to 873ab93 Compare October 29, 2024 17:52
@samlaf samlaf marked this pull request as ready for review October 29, 2024 19:18
@samlaf samlaf requested a review from bxue-l2 October 29, 2024 19:18
flags/eigendaflags/cli.go Show resolved Hide resolved
server/errors.go Outdated Show resolved Hide resolved
server/errors.go Show resolved Hide resolved
store/generated_key/eigenda/eigenda.go Outdated Show resolved Hide resolved
bxue-l2
bxue-l2 previously approved these changes Oct 30, 2024
flags/eigendaflags/cli.go Show resolved Hide resolved
server/errors.go Show resolved Hide resolved
@bxue-l2 bxue-l2 dismissed their stale review October 30, 2024 18:05

need more test

@samlaf samlaf force-pushed the samlaf/feat--503-error-logic-with-retries branch from f317339 to 727a63d Compare October 31, 2024 15:28
chore: go mod tidy to generate go.mod

feat: dealing with new eigenda-client grpc errors + ErrorFailover convention

comment: fix typo

feat(handlers): postShared returns 429 when disperser rate limited client

flag(eigenda): rename RetriesBeforeFailover -> PutRetries

reviewer correctly pointed out that retrying was more general than only for failovers

lint: nolint exhaustive switch check for Put case
@samlaf samlaf force-pushed the samlaf/feat--503-error-logic-with-retries branch from 9c6a735 to 574a762 Compare November 1, 2024 22:20
@samlaf samlaf requested a review from bxue-l2 November 2, 2024 00:47
Copy link
Collaborator

@bxue-l2 bxue-l2 left a comment

Choose a reason for hiding this comment

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

can't finish.

flags/eigendaflags/cli.go Show resolved Hide resolved
flags/eigendaflags/cli.go Outdated Show resolved Hide resolved
@@ -5,7 +5,8 @@ go 1.22
toolchain go1.22.0

require (
github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d
github.com/Layr-Labs/eigenda v0.8.5-rc.0.0.20241101212705-fa8776ae648c
Copy link
Collaborator

Choose a reason for hiding this comment

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

after core mainnet release, remember to change it into v0.8.5

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@bxue-l2 bxue-l2 left a comment

Choose a reason for hiding this comment

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

overall, lgtm. But didn't take closer look into the test, but it covered all the test

} else {
case is429(err):
http.Error(w, err.Error(), http.StatusTooManyRequests)
case errors.Is(err, &api.ErrorFailover{}):
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be namedis503(err)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

store/generated_key/eigenda/eigenda.go Show resolved Hide resolved
store/generated_key/eigenda/eigenda.go Show resolved Hide resolved
flags/eigendaflags/cli.go Show resolved Hide resolved
@samlaf samlaf requested a review from epociask November 7, 2024 14:51
Copy link
Collaborator

@bxue-l2 bxue-l2 left a comment

Choose a reason for hiding this comment

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

lgtm, please address Ethen's comment

Copy link
Collaborator

@epociask epociask left a comment

Choose a reason for hiding this comment

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

LGTM

@samlaf samlaf merged commit 9f04e56 into main Nov 11, 2024
7 checks passed
@samlaf samlaf deleted the samlaf/feat--503-error-logic-with-retries branch November 11, 2024 04:31
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.

4 participants