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.

Sign up for GitHub

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

Jump to bottom

A62: pick_first: sticky TRANSIENT_FAILURE and address order randomization #357

Merged
merged 7 commits into from
May 17, 2023
Merged

A62: pick_first: sticky TRANSIENT_FAILURE and address order randomization #357

merged 7 commits into from
May 17, 2023

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Apr 21, 2023 •
edited
Loading

No description provided.

@easwars
Copy link
Contributor Author

easwars commented Apr 21, 2023

@markdroth : Not sure if this is sort of what we are looking for in this gRFC. Also I decided to include the config knob (since deleting is easier) even though we are leaning in favor of doing the shuffling higher up the LB policy tree. Please let me know what you feel. I can rewrite based on that. Thanks.

@easwars easwars requested a review from markdroth April 21, 2023 06:58
markdroth
markdroth reviewed Apr 21, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth 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 doing this!

A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
@easwars easwars marked this pull request as ready for review April 25, 2023 23:37
This was referenced Apr 26, 2023
Closed
Merged
@markdroth markdroth changed the title A62: pick_first LB policy A62: pick_first: sticky TRANSIENT_FAILURE and address order randomization May 1, 2023
markdroth
markdroth reviewed May 1, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks really good! Comments are mostly cosmetic.

@ejona86, did you also want to review this?

A62-pick-first.md Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md
before attempting to reconnect.

When connections to all addresses fail, there are some similarities and some
differences between the Java/Go implementations and the C-core implementation.
Copy link
Member

Choose a reason for hiding this comment

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

What's the behavior in node? @murgatroid99

Copy link
Member

Choose a reason for hiding this comment

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

Node's current behavior is similar to Java and Go in regards to TF reporting. Node also does not currently implement IDLE_TIMEOUT.

markdroth
markdroth reviewed May 1, 2023
View reviewed changes
A62-pick-first.md Outdated Show resolved Hide resolved
ejona86
ejona86 reviewed May 1, 2023
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Looks fine. I'd be comfortable not re-reviewing after editorial changes.

A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
@ejona86
Copy link
Member

ejona86 commented May 1, 2023

We should probably explicitly say that implementations should implement IDLE_TIMEOUT and enabled by default. We can recommend 30 minutes for the default.

@easwars
Copy link
Contributor Author

easwars commented May 2, 2023

We should probably explicitly say that implementations should implement IDLE_TIMEOUT and enabled by default. We can recommend 30 minutes for the default.

Added a note for this. Thanks.

@easwars
Copy link
Contributor Author

easwars commented May 2, 2023

Yeah, I'd either write this as a problem and put it in the background section or move it to the rationale. Even with this moved to rationale, I do think we're seriously missing a problem statement early in the gRFC.

I've rewritten the sections by talking about the problem first and then proposing the solution. But I haven't changed the abstract. Let me know if this suffices, or you would like to see something more in the abstract. Thanks.

markdroth
markdroth reviewed May 2, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Sorry for the churn here -- I noticed a few more things to address.

Please let me know if you have any questions. Thanks!

A62-pick-first.md Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
@easwars
Copy link
Contributor Author

easwars commented May 3, 2023

Oops. I failed to the see the big comment because it did not appear on the "files" tab :(.
Addressing that now.

markdroth
markdroth reviewed May 3, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks great! Just a few minor comments remaining.

A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
A62-pick-first.md Show resolved Hide resolved
A62-pick-first.md Outdated Show resolved Hide resolved
@easwars easwars mentioned this pull request May 3, 2023
Merged
markdroth
markdroth reviewed May 3, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks great! I'm happy to approve at this point, but let's wait the 2 weeks to give a chance for community feedback.

Thanks for doing this!

@easwars
Copy link
Contributor Author

easwars commented May 17, 2023

@markdroth : Looks like it has been two weeks since I sent the email for this proposal to grpc.io@. Could we merge this now? Thanks.

@markdroth markdroth merged commit 15c94db into grpc:master May 17, 2023
@easwars easwars deleted the pick_first branch May 17, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejona86 ejona86 ejona86 left review comments

@murgatroid99 murgatroid99 murgatroid99 left review comments

@markdroth markdroth markdroth approved these changes

Assignees

@markdroth markdroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@easwars @ejona86 @murgatroid99 @markdroth