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: Avoid unnecessary status updates #637

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

morgsmccauley
Copy link
Collaborator

@morgsmccauley morgsmccauley commented Apr 4, 2024

Quick & dirty PR which short-circuits updating the status via GraphQL when it is unchanged. We don't need to check whether block height has changed, as that is updated in a separate call later on. I've also updated the integration tests to assert the output of status/logs.

@morgsmccauley morgsmccauley linked an issue Apr 4, 2024 that may be closed by this pull request
@morgsmccauley morgsmccauley force-pushed the feat/unnecessary-status-updates branch from 9fb704e to 42bb4b8 Compare April 7, 2024 22:46
@morgsmccauley morgsmccauley marked this pull request as ready for review April 7, 2024 22:49
@morgsmccauley morgsmccauley requested a review from a team as a code owner April 7, 2024 22:49
@darunrs
Copy link
Collaborator

darunrs commented Apr 8, 2024

I don't believe this would work for FAILING indexers. Since we write RUNNING, run the code, and when it fails, write FAILING. I suppose that's still fine since they're failing anyway, but I did want to call that out, if you have an idea on how to quickly cover that case too.

Copy link
Collaborator

@darunrs darunrs left a comment

Choose a reason for hiding this comment

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

Quick comment on failing indexers. Otherwise, should do the job!

@morgsmccauley
Copy link
Collaborator Author

I don't believe this would work for FAILING indexers. Since we write RUNNING, run the code, and when it fails, write FAILING. I suppose that's still fine since they're failing anyway, but I did want to call that out, if you have an idea on how to quickly cover that case too.

Good point, this only solves the happy path for when Indexers are continuously succeeding. In the failing case, we actually want to update: from RUNNING -> FAILING, and then FAILING -> RUNNING again. I guess the change would be to avoid setting it back to RUNNING until it has actually succeeded. But like you said, given they are failing we wouldn't be benefiting much.

I'd opt for moving forward with this, and waiting for your change to mitigate the above :)

@morgsmccauley morgsmccauley merged commit 86acb1e into main Apr 9, 2024
3 checks passed
@morgsmccauley morgsmccauley deleted the feat/unnecessary-status-updates branch April 9, 2024 00:50
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.

Remove Unnecessary Status Writes
2 participants