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

Add grisp.io custom protocol version header #53

Merged
merged 2 commits into from
Feb 20, 2025
Merged

Conversation

sylane
Copy link
Contributor

@sylane sylane commented Feb 17, 2025

Add grisp.io protocol versioning

  • Remove invalid configuration.
  • Fix dialyzer spec.
  • Add option to limit the number of connection retries, mostly for
    testing use-cases that will always fail to connect.
  • Implemente wait_connected in the client to support reaching maximum
    connection retries and get the last known error.

@sylane sylane requested review from maehjam and ziopio February 17, 2025 09:18
@sylane
Copy link
Contributor Author

sylane commented Feb 17, 2025

I can't reproduce the CT error :(

@sylane
Copy link
Contributor Author

sylane commented Feb 17, 2025

I forgot to push some changes in jarl...

@sylane sylane force-pushed the sylane/add-proto-version branch from 7e3790d to bc14c9e Compare February 17, 2025 13:39
 - Remove invalid configuration.
 - Fix dialyzer spec.
 - Add option to limit the number of connection retries, mostly for
   testing use-cases that will always fail to connect.
 - Implemente wait_connected in the client to support reaching maximum
   connection retries and get the last known error.
@sylane sylane force-pushed the sylane/add-proto-version branch from a5d38da to fb7616d Compare February 18, 2025 14:25
@@ -85,6 +90,11 @@ is_connected() ->
catch exit:noproc -> false
end.

wait_connected(Timeout) ->
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to adjust is_connected to include a timeout instead of adding yet another function for more or less the same feature? Is there a reason that we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_connected return true or false right away, it doesn't wait for the connection to either be established or to fail because the maximum number of retries has been reached. Maybe we don't need is_connected, but it is definitely not the same.

Copy link
Member

Choose a reason for hiding this comment

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

That was my question: Do we need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cover a different use-case, I needed to add wait_connected, but didn't want to change grisp_connect API that is exposing is_connected.

Copy link
Member

Choose a reason for hiding this comment

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

IMO is a nice function to have

test/grisp_connect_test_server.erl Show resolved Hide resolved
test/grisp_connect_test_server.erl Show resolved Hide resolved
test/grisp_connect_api_SUITE.erl Show resolved Hide resolved
@sylane sylane merged commit 062f320 into main Feb 20, 2025
1 check passed
@sylane sylane deleted the sylane/add-proto-version branch February 20, 2025 10:45
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.

3 participants