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

[WIP] Re-Implement Monitors #64

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

mrpalide
Copy link
Contributor

Fixes: #63

Changes:

  • reimplement dmsg-monitor
  • reimplement ar-monitor
  • combine dmsg-monitor and ar-monitor as network-monitor
  • reimplement tpd-monitor

How to test:

  • run integration-env test, kill one visor, check network-monitor log
  • TBD [for tpd-monitor]

@0pcom
Copy link
Collaborator

0pcom commented Apr 10, 2024

Can you describe briefly the mechanism by which this is supposed to work?

it looks to me as though the uptime tracker is used in some way to remove dead entries for dmsg discovery.

I believe in the previous implementation, you were attempting to make transports to each of those keys in dmsg discovery with the dmsg monitor.

Both of those methods are incorrect for dmsg-discovery. The only option for dmsg may be to implement a heartbeat for dmsg discovery. In that way, the client re-registers itself and any keys may just be expired once a certain time has passed without re-registration.

One can run a dmsg client for any purpose - separately from the visor ; or even for no purpose. In that instance, there is no endpoint that can be checked externally to ensure that it is actually online, and our only concern is to have a feedback mechanism which will prevent the database of entries from expanding endlessly.

This is also the case for any long-lived dmsghttp client such as dmsgweb. There is nothing that you can check on an http client externally to see if it's online because it's supposed to interact with a server.

Unless my analysis is flawed, I suggest we drop the dmsg monitor altogether at this point.

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