-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[CRE-1028] Optimize beholder validator in system tests: structured init and message handling (part 2) #19723
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
Conversation
7d122aa to
f28ae23
Compare
f28ae23 to
d543f09
Compare
Tofel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proto registration retry is and should be handled in the CTF, not here.
core/scripts/cre/environment/examples/workflows/v2/proof-of-reserve/cron-based/main.go
Show resolved
Hide resolved
| sleepDuration := backoff + jitter | ||
| if sleepDuration > maxBackoffTimeout { | ||
| sleepDuration = maxBackoffTimeout | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use avast's retry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this specific case, I have several concerns, as I am uncertain whether the retry library can handle the following:
- When using
retry.Do(), we need to carefully determine when to close the output channel (e.g.defer close(out)) - only after all retries are exhausted or the context is canceled. - A jitter feature to prevent thundering herd problems will be lost, along with some essential debug logging.
- The
readyChmust be carefully managed - it should only signal once, at the first successful connection, not on every retry. This requires state tracking outside the retry function.
049cd4e to
b3026d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements part 2 of the Beholder validator optimization in system tests, focusing on enhanced reliability, error handling, and message processing. It significantly restructures the Kafka consumer implementation to address timer leaks, improve connection handling, and add comprehensive validation.
- Enhanced Kafka consumer with exponential backoff, heartbeat validation, and structured error handling
- Added fail-fast validation for Beholder subscription errors during initialization
- Updated dependencies and improved documentation/comments
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| system-tests/tests/test-helpers/t_helpers.go | Added fail-fast error checking for Beholder subscription initialization |
| system-tests/tests/test-helpers/beholder_provider.go | Complete restructure with improved Kafka consumer, validation, and error handling |
| system-tests/tests/regression/cre/v2_evm_regression_test.go | Fixed typo and increased timeout duration |
| system-tests/tests/regression/cre/cre_regression_suite_test.go | Updated documentation comments |
| system-tests/tests/go.mod | Moved retry-go dependency from indirect to direct |
| go.md | Added dependency relationship for chainlink-evm/gethwrappers |
| core/scripts/go.mod | Moved retry-go dependency from indirect to direct |
| core/scripts/cre/environment/examples/workflows/v2/proof-of-reserve/cron-based/main.go | Replaced hardcoded ABI with generated bindings |
| core/scripts/cre/environment/examples/workflows/v2/proof-of-reserve/cron-based/go.mod | Added chainlink-evm/gethwrappers dependency |
| core/scripts/cre/environment/environment/beholder.go | Added retry logic for protobuf registration |
| .changeset/slick-drinks-like.md | Changeset for PoR workflow binding updates |
| .changeset/eight-tips-bathe.md | Changeset for Beholder optimization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/scripts/cre/environment/examples/workflows/v2/proof-of-reserve/cron-based/main.go
Show resolved
Hide resolved
core/scripts/cre/environment/examples/workflows/v2/proof-of-reserve/cron-based/main.go
Show resolved
Hide resolved
b3026d4 to
10805b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…it and message handling (part 2) Add retries with exponential backoff to a proto registration function Refactor beholder validator Add Kafka listener reconnection when UserLogs are empty within timeout Add Beholder heartbit and consumer connectivity validation before starting system tests Add Beholder heartbit and consumer connectivity validation before starting system tests
bf55c1c to
439c04b
Compare
439c04b to
c955503
Compare
|




CRE-1028: Optimize beholder validator in system tests: structured init and message handling (part 2)
In
beholder.go:In
beholder_provider.go: