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

fix: skip PublisherStalledCheck if PublisherOfflineCheck fails #71

Merged
merged 3 commits into from
May 20, 2024

Conversation

cctdaniel
Copy link
Contributor

@cctdaniel cctdaniel commented May 17, 2024

we agree that PublisherStalledCheck should fulfil these 3 requirements:

  • should be skipped when market hours is closed
  • should be skipped when publisher is offline (PublisherOfflineCheck fails)
  • should be skipped when PublisherOfflineCheck returns true but distance > self.__abandoned_slot_distance

there seems to be 2 ways we can do this:

  • duplicating the offline check logic in PublisherStalledCheck including modifying the check to accept max_slot_distance and abandoned_slot_distance fields which don't really seem to belong there since this is for PublisherOfflineCheck
  • make sure PublisherStalledCheck runs after PublisherOfflineCheck and keep track of the deps with flags

instead of modifying PublisherStalledCheck to also accept max_slot_distance and abandoned_slot_distance solely for the purpose of replicating the offline check, I have chosen the second option in this PR which is to modify check_publisher() so that we don't have to duplicate the offline check, this is definitely not without its drawback, which is that now the check_publisher() function becomes more complicated

update: previously I've tried to approach this issue using option 2 but it turns out changes the code to be more complicated and introduce dependencies that will make it more unmaintainable so I have reverted to the first option instead

@cctdaniel cctdaniel closed this May 20, 2024
@cctdaniel cctdaniel force-pushed the stalled-check-offline branch from 212e656 to d8ca2b6 Compare May 20, 2024 03:08
@cctdaniel cctdaniel reopened this May 20, 2024
@cctdaniel cctdaniel merged commit 5cd502f into main May 20, 2024
3 checks passed
@cctdaniel cctdaniel deleted the stalled-check-offline branch May 20, 2024 13:54
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.

2 participants