Skip to content

Implement a retry mechanism for VAST clients failing to connect to the server #2835

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

Merged
merged 7 commits into from
Jan 23, 2023

Conversation

patszt
Copy link
Contributor

@patszt patszt commented Jan 4, 2023

  1. Added a new actor that allows the caller to connect with a remote node in async/sync manner with a retry mechanism.
  2. Adjusted current connect_to_node to use the new actor as a implementation detail.

I have done a simple test to check the retry mechanism.
Run export -> wait a 2 - 3 seconds and run vast start.
Previously the client exits with error:

! system_error: failed to connect to VAST node at 127.0.0.1:42000: !! caf::sec::cannot_connect_to_node: ip_connect failed 127.0.0.1 message("ip_connect failed", "127.0.0.1", 42000)

Now it manages to export the data.

@patszt patszt added the feature New functionality label Jan 4, 2023
mavam
mavam previously requested changes Jan 4, 2023
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Everything is there, nice. I think we can gut out the code a bit to make it simpler.

@mavam
Copy link
Member

mavam commented Jan 5, 2023

@patszt can you please update the user-facing docs as well at the section where it would make sense?

@patszt patszt marked this pull request as ready for review January 9, 2023 10:04
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Some minor requests, and then I think we soon got it. 🚀

@patszt patszt marked this pull request as draft January 17, 2023 15:48
@patszt patszt force-pushed the story/sc-39940 branch 5 times, most recently from 9e83d56 to 84cfc35 Compare January 18, 2023 11:38
@patszt patszt marked this pull request as ready for review January 18, 2023 11:40
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This works really nicely in practice. Just some really minor stuff remaining. Great work, Patryk! 🙏

@dominiklohmann dominiklohmann dismissed mavam’s stale review January 20, 2023 09:13

Request was addressed.

@patszt patszt merged commit 1a31d58 into master Jan 23, 2023
@patszt patszt deleted the story/sc-39940 branch January 23, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants