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

java: modify Java example to move healthService to the admin server #45

Merged

Conversation

sanjaypujare
Copy link
Collaborator

No need to have both health and admin server. So we just add health service to the admin server

CC @ericgribkoff @yashykt @easwars @lidizheng

As per the discussion

@ericgribkoff
Copy link
Contributor

Doesn't our documentation describe creating health checks using the gRPC protocol and the serving port? It looks like moving the health server to a non-serving port would break e.g. https://cloud.google.com/traffic-director/docs/set-up-proxyless-gce#creating_the_health_check_firewall_rule_and_backend_service without an accompanying update to the documentation

@sanjaypujare
Copy link
Collaborator Author

Doesn't our documentation describe creating health checks using the gRPC protocol and the serving port? It looks like moving the health server to a non-serving port would break e.g. https://cloud.google.com/traffic-director/docs/set-up-proxyless-gce#creating_the_health_check_firewall_rule_and_backend_service without an accompanying update to the documentation

Eric, nothing will break. There is also (old code, check https://github.com/GoogleCloudPlatform/traffic-director-grpc-examples/blob/master/java/src/main/java/io/grpc/examples/wallet/StatsServer.java#L179) a health-service on the serving port which is not being removed. The only change is to not create a separate plaintext health-check only port but to overload the admin port to also be the plaintext health check port.

@sanjaypujare
Copy link
Collaborator Author

Friendly ping. Any comments? Can this be approved?

Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

The change looks good. I understand that the UHC can't use xDS credentials. Exposing additional health check port sounds good to me.

Please manually trigger https://fusion.corp.google.com/projectanalysis/summary/KOKORO/prod:grpc%2Fwallet-example%2Fmaster%2Fbranch%2Fgrpc_wallet

If possible, please follow the tutorial one more time with this PR https://cloud.google.com/traffic-director/docs/security-proxyless-setup
(Though I don't see breaking changes.)

export EXAMPLES_VERSION=admin-port-mod
export EXAMPLES_REPO=sanjaypujare

@sanjaypujare sanjaypujare merged commit 6a204dc into GoogleCloudPlatform:master Jun 16, 2021
@sanjaypujare sanjaypujare deleted the admin-port-mod branch June 16, 2021 18:00
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.

None yet

3 participants