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

[Doc] better define the hosts selection policy #32

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Aug 24, 2021

This commit update the description of the hosts setting

@andsel andsel requested a review from karenzone August 24, 2021 12:32
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Thanks for adding this info to explain how hosts selection work. It includes sentence that isn't quite clear to me. Perhaps we can reword or elaborate.

You are a good explainer. If you explain it to me, I can help with the right wording.

Comment on lines 64 to 66
List of addresses lumberjack can send to. When the plugins needs to connect to the remote
peer it randomly selects one of the hosts. The connection happens when the plugin is registered
and when it detects a connection error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
List of addresses lumberjack can send to. When the plugins needs to connect to the remote
peer it randomly selects one of the hosts. The connection happens when the plugin is registered
and when it detects a connection error.
List of addresses lumberjack can send to. When the plugin needs to connect to the remote
peer, it randomly selects one of the hosts. The connection happens when the plugin is registered
and when it detects a connection error.

Copy link
Contributor

Choose a reason for hiding this comment

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

"The connection happens when the plugin is registered
and when it detects a connection error."

This sentence seems counter intuitive. What is it about the connection error that makes the connection happen?

Maybe if one connection fails, it moves to another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @karen to look at this, I try to explain the logic flow. When the pipeline is created and the plugin registered it open a connection to one of the hosts. When the plugin is sending data and receives an IO error (due to network or host problems) is opens a freshly connection, bringing the new host from the hosts list. How the plugin selects the host to connect, is always in a random way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! Great explanation.
Let's make it clear that we're covering two different scenarios--happy path and failure.

What about something like:

When the plugin is registered, it opens a connection to one of the hosts.
If the plugin detects a connection error, it selects a different host from the list and opens a new connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better, but we need to say that the selection from the host list is random, because the original point that generated this PR was "how the host is selected from the hosts list?".

Maybe something like:

When the plugin is registered, it opens a connection to one of the hosts.
If the plugin detects a connection error, it selects a different host from the list and opens a new connection.
The host is selected in randomly fashion.

Copy link
Contributor

Choose a reason for hiding this comment

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

You've already got that info covered in Lines 64-65.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh ok, I thought your phrase was a complete replacement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenzone I've integrated your suggestion and bumped a new version to publish the documentation changes

@andsel andsel requested a review from karenzone August 26, 2021 12:20
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Nice doc enhancement. Thank you! 🎉
I left some very minor suggestions inline (adding a comma and a return) for your consideration. Otherwise, LGTM!

I trust your judgement and don't need to see it again, unless you want me to for some reason.

docs/index.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Karen Metts <[email protected]>
@andsel andsel merged commit de7106f into master Aug 30, 2021
@karenzone
Copy link
Contributor

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