Skip to content

Conversation

zhangkun83
Copy link
Contributor

The condition "effectiveServiceConfig != validServiceConfig" should
have been deleted in commit 2162ad0.

The condition was there before that commit because
NAME_RESOLVER_SERVICE_CONFIG was already in "attrs", thus it needed to
be re-added only if "effectiveServiceConfig" differs from the original
"validServiceConfig".

In contrast, ATTR_HEALTH_CHECKING_CONFIG is not in the original
"attrs" and always needs to be added.

The condition "effectiveServiceConfig != validServiceConfig" should
have been deleted in commit 2162ad0.

The condition was there before that commit because
NAME_RESOLVER_SERVICE_CONFIG was already in "attrs", thus it needed to
be re-added only if "effectiveServiceConfig" differs from the original
"validServiceConfig".

In contrast, ATTR_HEALTH_CHECKING_CONFIG is not in the original
"attrs" and always needs to be added.
@zhangkun83 zhangkun83 requested review from creamsoup and ejona86 March 5, 2020 01:58
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.

Arg. I was watching that condition earlier, but then didn't check it in the end...

@zhangkun83 zhangkun83 merged commit 4a2c5d6 into grpc:master Mar 5, 2020
@zhangkun83 zhangkun83 deleted the health_check_fix branch March 5, 2020 17:28
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
The condition "effectiveServiceConfig != validServiceConfig" should
have been deleted in commit 2162ad0.

The condition was there before that commit because
NAME_RESOLVER_SERVICE_CONFIG was already in "attrs", thus it needed to
be re-added only if "effectiveServiceConfig" differs from the original
"validServiceConfig".

In contrast, ATTR_HEALTH_CHECKING_CONFIG is not in the original
"attrs" and always needs to be added.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants