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

[SHIP-3903]Remove WS requirement for data feeds soak tests #15003

Merged
merged 10 commits into from
Nov 5, 2024

Conversation

davidcauchi
Copy link
Contributor

@davidcauchi davidcauchi commented Oct 29, 2024

SHIP-3903

Switch from subscription to polling for logs to remove the requirement of having a WS endpoint.

kalverra
kalverra previously approved these changes Oct 29, 2024
joaoluisam
joaoluisam previously approved these changes Oct 29, 2024
Copy link
Contributor

@joaoluisam joaoluisam left a comment

Choose a reason for hiding this comment

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

LGTM


// Track last checked block number and update starting block number
lastCheckedBlockNum = latestBlock
startingBlockNum = latestBlock + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: can we just repurpose startingBlockNum to check for the lastCheckedBlockNum. Not a big deal but good for readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Wanted to have a clear separation between the initial run and subsequent runs but it does not add that much value. Simpler code is always better.

for {
select {
case event := <-eventLogs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think its worth keeping the WSS option also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would move onto polling. Seems more reliable and not prone to disconnections.

simsonraj
simsonraj previously approved these changes Oct 30, 2024
@davidcauchi davidcauchi requested a review from a team as a code owner October 30, 2024 11:15
func (o *OCRSoakTest) pollingOCREvents(endTest <-chan time.Time) error {

// Keep track of the last processed block number
processedBlockNum := o.startingBlockNum - 1

go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope, but this goroutine appears to be untracked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated how this is handled and it is now tracked. Thanks for pointing it out.


go func() {
// TODO: Make this configurable
Copy link
Contributor

Choose a reason for hiding this comment

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

For this PR or follow-up? Is there a ticket to link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intention was for a follow up. Just created SHIP-3973 to cover it.

@kalverra kalverra added this pull request to the merge queue Nov 5, 2024
Merged via the queue into develop with commit 0b38fca Nov 5, 2024
141 of 142 checks passed
@kalverra kalverra deleted the ship-3903-remove-ws-from-df-tests branch November 5, 2024 18:10
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.

5 participants