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

Return the health server when setting up #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GTB3NW
Copy link

@GTB3NW GTB3NW commented Jan 22, 2021

Allows for a DRY approach to setting up a re-usable health server. My use case is that I want to serve some services as healthy when they're read-only, therefore allowing a single connection to be used for multiple clients. If you think I should take a different approach feel free to correct me :)

@Jille
Copy link
Owner

Jille commented Jan 22, 2021

Hi Ben!

Thanks for your PR. I don't think I understand what you're trying to achieve yet. Are you trying to avoid having to import google.golang.org/grpc/health/grpc_health_v1 in your calling code? That seems reasonable.

So far my idea for the Setup() function is that it's a convenience wrapper, and people wanting to do more difficult things can use Report() directly. If we're going to return *health.Server from Setup(), we should probably document that most callers should ignore it.

@GTB3NW
Copy link
Author

GTB3NW commented Jan 25, 2021

Hi Ben!

Hello!

Thanks for your PR. I don't think I understand what you're trying to achieve yet. Are you trying to avoid having to import google.golang.org/grpc/health/grpc_health_v1 in your calling code? That seems reasonable.

No worries, thank you for open sourcing! Sorry I explained badly. No we're not trying to avoid the import of that package, I'll address the change in the next quote. As for what we're trying to achieve...

So we want all writes to only go to the raft leader, which you package currently achieves very nicely. We'd also like to send all reads to followers, thereby offloading the burden from the leader. Exposing the health pointer allows us to piggy back of the existing health and add some additional services which we don't want the leaderhealth package to control the health for (the followers). If we reuse the health pointer, it means we can put all services on a single port, albeit we would need separate read/write connections on the clientside. To clarify, the services are split into read/write, only RPC's which write data will be in the write services and there will be a sister read service for anything which reads.

So far my idea for the Setup() function is that it's a convenience wrapper, and people wanting to do more difficult things can use Report() directly. If we're going to return *health.Server from Setup(), we should probably document that most callers should ignore it.

That's currently what I'm doing, I've replicated the setup function. The only thing the setup function wasn't doing which I needed was returning the pointer so I could add additional services. Sure the PR will break the signature and might require a slight version bump but I personally think it's worth it for re-usability. Hopefully that makes sense?

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