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

Extend documentation for SDKs #125

Merged
merged 1 commit into from
Mar 2, 2018
Merged

Conversation

markmandel
Copy link
Member

This includes updates for the fact we will have releases soon, as well as extending the documentation for the C++ SDK, as that will likely be the most popular.

@markmandel markmandel added kind/documentation Documentation for Agones area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Mar 2, 2018
@markmandel markmandel added this to the 0.1 milestone Mar 2, 2018
@markmandel
Copy link
Member Author

@Kuqd is there anyone on your end (you?) who might be good to look at this? I figure you have more experience with using the SDK than anyone else, and may have good bits to add / remove / etc?

@cyriltovena
Copy link
Collaborator

cyriltovena commented Mar 2, 2018

@sylvainduchesne for sure will have good feedback to give, as he worked on that part mostly.
I will for sure review it also.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@sylvainduchesne
Copy link

I will make some time today to go over the documentation as it stands.
However, I was wondering what was the intention for the health check?

To send a health check ping call sdk->Health(). This is a synchronous request that will return false if it has failed in any way.

If false is returned, should the process abort?
Should the health check originate from the server rather than the game server instance? Of course, this would imply having a callback mechanism on the sdk-side to respond the health query.

@markmandel markmandel force-pushed the doc/remove-yet-to-be-released branch from ca6cfa8 to 4ba9e4a Compare March 2, 2018 15:41
@googlebot
Copy link

CLAs look good, thanks!

@markmandel
Copy link
Member Author

@sylvainduchesne, thanks for your review! As an FYI I've rebased this down to a single commit again - but thanks for catching the semicolon!

Should the health check originate from the server rather than the game server instance? Of course, this would imply having a callback mechanism on the sdk-side to respond the health query.

This is by design. See #15 for original design and the health checking docs for more details. That being said, if you have thoughts on how health checking can be improved, definitely start a new ticket with your ideas. The SDK was deliberately left as simple as possible so that we could add functionality to it as we found it was necessary.

But this also tells me that this doc needs to link back to the existing SDK + Health checking documentation, so I'll make those changes today as well.

@markmandel markmandel force-pushed the doc/remove-yet-to-be-released branch 2 times, most recently from 6e3cfb9 to b284db0 Compare March 2, 2018 16:43
@markmandel
Copy link
Member Author

Updated with more links, and explanation of failure conditions (basically, it shouldn't). PTAL!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@sylvainduchesne
Copy link

So I read through the entire doc, I haven't seen any pre-built binaries (refering to https://github.com/googleprivate/agones/blob/doc/remove-yet-to-be-released/sdks/README.md#local-development), but I imagine this will come along with an actual release?

Coming back to the health check having agones periodically poke the game server instances would make more sense, considering that it needs to be pinged in a specific interval to not be marked unhealthy. It's one more piece of information that the GameServer requires to know (that is, how often it is required to ping to not be marked unhealthy).
I guess the flip-side to allow the process decide how to proceed in an unhealthy state could be beneficial (not terminate a game session in case of a false negative), but couldn't it also cause game server instance leaks (say the process is really crashed)?

I can open an issue to discuss it further. :)

@markmandel
Copy link
Member Author

Yep, the release is coming next week, so we'll have the files there then!

Yes - please definitely open a ticket, and we can hash it out 👍 sounds like it will be a valuable conversation, you've raised some good points, particularly around the gameserver needing to know how often an unhealthy state is defined, but I have some ideas on how we can solve those 👍

Thanks for looking though - will rebase back down again shortly.

@markmandel markmandel force-pushed the doc/remove-yet-to-be-released branch from 4c7a7df to 74758a8 Compare March 2, 2018 19:54
@googlebot
Copy link

CLAs look good, thanks!

This includes updates for the fact we will
have releases soon, as well as extending the
documentation for the C++ SDK, as that will likley
be the most popular.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/documentation Documentation for Agones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants