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: add additional clause for Telem.client_join #219

Merged
merged 16 commits into from
Jan 5, 2024

Conversation

J0
Copy link
Contributor

@J0 J0 commented Dec 11, 2023

What kind of change does this PR introduce?

Fix #214.

Omitted a test on this one since it seems like we don't test telem.ex

@J0 J0 marked this pull request as ready for review December 12, 2023 16:47
@J0
Copy link
Contributor Author

J0 commented Dec 12, 2023

Actually on further thought maybe we should write a test. Will do so tomorrow

@J0 J0 marked this pull request as draft December 12, 2023 16:59
@J0 J0 marked this pull request as ready for review December 19, 2023 08:36
@J0
Copy link
Contributor Author

J0 commented Dec 19, 2023

Found it a little hard to write a test so I tested it manually using telnet

It now returns

16:32:42.351 [error] Client startup message error: :bad_startup_payload file=lib/supavisor/client_handler.ex line=149 pid=<0.1056.0>
16:32:42.351 [warning] client_join is called with a mismatched id: nil file=lib/supavisor/monitoring/telem.ex line=61 pid=<0.1056.0>

CleanShot 2023-12-19 at 16 36 32@2x

@J0 J0 requested a review from abc3 December 19, 2023 08:37
@J0
Copy link
Contributor Author

J0 commented Jan 4, 2024

Hey @abc3

Sorry to bother you again - just going through list of existing open PRs to close them out. Any chance I could get a review on this again when free?

Here's how it was tested:

  1. run docker compose -f docker-compose-db.yml up
  2. run make dev
  3. run telnet localhost 6543 and then enter a foreign character (e.g. q). See the error message (as below)
CleanShot 2024-01-04 at 14 19 57@2x

Lmk if there's more I should add or more cases that we should handle

@abc3 abc3 merged commit 694ff2a into main Jan 5, 2024
2 checks passed
@abc3 abc3 deleted the j0/fix_telem_client_join branch January 5, 2024 11:14
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.

Telem.client_join clause error in ClientHandler
2 participants