Implement block on connect with Rust+Node.js SDK #953

markmandel · 2019-07-27T07:12:14Z

Rust

Uses a simple polling mechanism to confirm the connection is live
Add ready to the SDK documentation.

Node.js

Implements a sdk.connect() function

Make

Tweaks the conformance tests to add a random delay to ensure future
testing of SDK blocking.

Fixes #938

markmandel · 2019-07-27T07:12:33Z

build/includes/sdk.mk

-	sleep 2s && DOCKER_RUN_ARGS="--network=host $(DOCKER_RUN_ARGS)" COMMAND=sdktest $(MAKE) run-sdk-command & \
-	docker run -p 59357:59357 -e "ADDRESS=" \
+	DOCKER_RUN_ARGS="--network=host $(DOCKER_RUN_ARGS)" COMMAND=sdktest $(MAKE) run-sdk-command & \
+	sleep $(DELAY) && docker run -p 59357:59357 -e "ADDRESS=" \


@aLekSer wdyt of this approach?

yes, this one much better

markmandel · 2019-07-27T07:13:28Z

sdks/rust/src/sdk.rs

@@ -43,7 +46,24 @@ impl Sdk {
            .connect(&addr);
        let cli = sdk_grpc::SdkClient::new(ch);
        let req = sdk::Empty::new();
-        let _ = cli.ready(&req).map(Box::new)?;
+


@thara would love your review of my Rust code. I'm a little.... rusty 🙄 🤣 🦀

agones-bot · 2019-07-27T07:40:32Z

Build Failed 😱

Build Id: 8d25b56d-ccaf-4288-aaa3-5a05315eb41c

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

site/content/en/docs/Guides/Client SDKs/rust.md

agones-bot · 2019-07-27T11:03:59Z

Build Failed 😱

Build Id: 1f485ad6-2ece-44b8-939b-e45e44027942

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel · 2019-07-28T02:43:25Z

Hmnn. Not sure why we are failing. Will look into it. I think it's failing on the nodejs version - not sure I'm following the logs well.

markmandel · 2019-07-28T08:31:34Z

Yep, can confirm - it's the node client - I assumed it blocked on connect, but it doesn't look like it. I'm hunting it down by @steven-supersolid if you have the code needed to block on connect, that would be awesome.

markmandel · 2019-07-28T08:35:57Z

https://grpc.github.io/grpc/node/grpc.Client.html#waitForReady__anchor looks like what I need, but now trying to work out how to apply it in an async language :D

markmandel · 2019-07-28T09:06:50Z

Here's my attempt:
https://github.com/googleforgames/agones/pull/953/files#diff-a7b304ad3eaf389b05bfeb118f316989R27

But I keep getting:

attempting to connect
Error: Failed to connect before the deadline
    at checkState (/go/src/agones.dev/agones/sdks/nodejs/node_modules/grpc/src/client.js:833:16)

But it doesn't look like it actually waiting for the deadline. Not quite sure why it isn't waiting.

markmandel · 2019-07-28T09:23:10Z

Worked it out - but I should write some docs on it 👍 (hopefully it passes). Would still love a review of my node.js though. I'll update my PR description too.

agones-bot · 2019-07-28T12:40:21Z

Build Failed 😱

Build Id: bea5726c-71ac-4e16-bba0-cae5dbce6ed6

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

steven-supersolid · 2019-07-28T20:57:47Z

sdks/nodejs/src/agonesSDK.js

+
+	async connect() {
+		return new Promise((resolve, reject) => {
+			this.client.waitForReady((new Date).getTime() + 30000, (error) => {


Minor but Date.now() is the more succinct way of getting the current system time. So Date.now() + 30000

steven-supersolid · 2019-07-28T21:00:08Z

sdks/nodejs/src/agonesSDK.js

@@ -23,7 +23,20 @@ class AgonesSDK {
 		this.healthStream = undefined;
 		this.emitters = [];
 	}
-	async close(){
+
+	async connect() {


We should add a test for this to maintain code coverage. Please let me know if you'd like me to add, e.g. separate PR?

Not sure how we are going to make a unit test for this, when there is nothing to connect to, but I will have a look, see if I can work out a way.

Aha, got it - there is a mocking library you use to mock out the client. On it!

Done! PTAL.

steven-supersolid · 2019-07-28T21:03:43Z

sdks/nodejs/src/agonesSDK.js

+
+	async connect() {
+		return new Promise((resolve, reject) => {
+			this.client.waitForReady((new Date).getTime() + 30000, (error) => {


I would be tempted to add the timeout to the constructor with a default value. E.g.

constructor(timeout = 30000) { this.timeout = timeout ... use later as Date.now() + this.timeout

Or if you like pass in as seconds and multiply by 1000 in the constructor

All other SDKs timeout by 30 seconds - we control the SDK server, so not personally seeing a good reason to make this configurable. I can't think of a use case in which someone would want to make this larger/smaller?

steven-supersolid · 2019-07-28T21:11:44Z

sdks/nodejs/src/agonesSDK.js

@@ -23,7 +23,20 @@ class AgonesSDK {
 		this.healthStream = undefined;
 		this.emitters = [];
 	}
-	async close(){
+
+	async connect() {


We should also update the docs.
I will look at generating API docs automatically for Node.js soon (maybe next weekend)

Yep - on my todo list.

Done!. PTAL.

agones-bot · 2019-07-29T04:14:09Z

Build Succeeded 👏

Build Id: 2cc0b4c5-5037-4f44-8d69-451ad7253888

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.12.0-76bc0c3
image: gcr.io/agones-images/agones-sdk:0.12.0-76bc0c3
image: gcr.io/agones-images/agones-ping:0.12.0-76bc0c3
Linux C++ SDK (build): agonessdk-0.12.0-76bc0c3-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.12.0-76bc0c3.zip

A preview of the website (the last 30 builds are retained):

https://76bc0c3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/953/head:pr_953 && git checkout pr_953
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-76bc0c3

agones-bot · 2019-07-29T05:58:11Z

Build Succeeded 👏

Build Id: 480575f8-f3ed-48ab-8a38-0a4801985710

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.12.0-22cd467
image: gcr.io/agones-images/agones-sdk:0.12.0-22cd467
image: gcr.io/agones-images/agones-ping:0.12.0-22cd467
Linux C++ SDK (build): agonessdk-0.12.0-22cd467-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.12.0-22cd467.zip

A preview of the website (the last 30 builds are retained):

https://22cd467-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/953/head:pr_953 && git checkout pr_953
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-22cd467

steven-supersolid · 2019-07-29T08:47:21Z

site/content/en/docs/Guides/Client SDKs/nodejs.md


 ```javascript
 const AgonesSDK = require('agones');

 let agonesSDK = new AgonesSDK();
 ```

+{{% feature publishVersion="0.12.0" %}}
+To connect to the SDK server, either local or when running on Agones, run the `sdk.connect()` method.
+This will block for up to 30 seconds if the SDK server has not yet started. If the connection cannot be made,


Minor but would call out that it's an async method that will resolve once connected or reject on error or if no connection can be made after 30 seconds. Technically it will block execution of the calling code if that uses await but does not block the Node.js event loop. I try to stray away from using the term block for async code and would reserve that for synchronous code that does block the event loop e.g. heavy calculation/loop

Thanks! Would definitely like to use more canonical language for node.js - will make this change.

steven-supersolid · 2019-07-29T08:51:16Z

I just realised we will also need to update the example in examples/nodejs-simple/src/index.js

markmandel · 2019-07-30T00:52:53Z

I just realised we will also need to update the example in examples/nodejs-simple/src/index.js

Yep - I think we can do this in a separate PR though - I think this one has enough in it already.

agones-bot · 2019-07-30T06:00:31Z

Build Succeeded 👏

Build Id: 90c29092-06b5-4e7b-9e25-17b661b27e58

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.12.0-2904081
image: gcr.io/agones-images/agones-sdk:0.12.0-2904081
image: gcr.io/agones-images/agones-ping:0.12.0-2904081
Linux C++ SDK (build): agonessdk-0.12.0-2904081-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.12.0-2904081.zip

A preview of the website (the last 30 builds are retained):

https://2904081-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/953/head:pr_953 && git checkout pr_953
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-2904081

roberthbailey · 2019-07-30T06:34:49Z

sdks/nodejs/src/agonesSDK.js

 	async close() {
-		if (this.healthStream !== undefined) {
+		if (this.healthStream !== undefined){


nit: you seem to have lost a space on this line

ARG! Fixing.

agones-bot · 2019-07-30T06:55:00Z

Build Succeeded 👏

Build Id: 7bf53655-a6ab-4a37-b5c5-477af5a02a8d

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.12.0-369db9b
image: gcr.io/agones-images/agones-sdk:0.12.0-369db9b
image: gcr.io/agones-images/agones-ping:0.12.0-369db9b
Linux C++ SDK (build): agonessdk-0.12.0-369db9b-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.12.0-369db9b.zip

A preview of the website (the last 30 builds are retained):

https://369db9b-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/953/head:pr_953 && git checkout pr_953
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-369db9b

- Uses a simple polling mechanism to confirm the connection is live - Add `ready` to the SDK documentation. - Implements and documents a sdk.connect() function - Tweaks the conformance tests to add a random delay to ensure future testing of SDK blocking. Fixes googleforgames#938

agones-bot · 2019-07-30T07:28:39Z

Build Succeeded 👏

Build Id: b7a57a5c-d926-400f-8301-012d6811e6ac

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.12.0-46d75e4
image: gcr.io/agones-images/agones-sdk:0.12.0-46d75e4
image: gcr.io/agones-images/agones-ping:0.12.0-46d75e4
Linux C++ SDK (build): agonessdk-0.12.0-46d75e4-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.12.0-46d75e4.zip

A preview of the website (the last 30 builds are retained):

https://46d75e4-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/953/head:pr_953 && git checkout pr_953
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-46d75e4

roberthbailey · 2019-07-30T07:41:59Z

@steven-supersolid & @thara - I'm going to merge this so that it gets in before the 0.12.0 release candidate cut. If you have any other review comments, feel free to send a followup PR to fix things up.

markmandel added kind/bug These are bugs. area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc area/tests Unit tests, e2e tests, anything to make sure things don't break labels Jul 27, 2019

markmandel commented Jul 27, 2019

View reviewed changes

roberthbailey reviewed Jul 27, 2019

View reviewed changes

site/content/en/docs/Guides/Client SDKs/rust.md Outdated Show resolved Hide resolved

markmandel force-pushed the bug/rust-sdk-block branch from fe0630d to 7faca63 Compare July 28, 2019 09:04

markmandel force-pushed the bug/rust-sdk-block branch from 7faca63 to 76bc0c3 Compare July 28, 2019 12:15

steven-supersolid reviewed Jul 28, 2019

View reviewed changes

markmandel changed the title ~~Implement block on connect with Rust SDK~~ Implement block on connect with Rust+Node.js SDK Jul 29, 2019

markmandel force-pushed the bug/rust-sdk-block branch 2 times, most recently from 4717b2c to 22cd467 Compare July 29, 2019 05:25

markmandel mentioned this pull request Jul 29, 2019

Updates to the rust simple example. #937

Merged

steven-supersolid reviewed Jul 29, 2019

View reviewed changes

markmandel force-pushed the bug/rust-sdk-block branch 2 times, most recently from f863909 to 2904081 Compare July 30, 2019 05:26

markmandel force-pushed the bug/rust-sdk-block branch from 2904081 to 369db9b Compare July 30, 2019 06:21

roberthbailey approved these changes Jul 30, 2019

View reviewed changes

markmandel force-pushed the bug/rust-sdk-block branch from 369db9b to 46d75e4 Compare July 30, 2019 06:55

roberthbailey approved these changes Jul 30, 2019

View reviewed changes

roberthbailey merged commit 06ffbd3 into googleforgames:master Jul 30, 2019

markmandel deleted the bug/rust-sdk-block branch July 30, 2019 07:52

markmandel added this to the 0.12.0 milestone Aug 1, 2019

markmandel added the kind/breaking Breaking change label Aug 1, 2019

Implement block on connect with Rust+Node.js SDK #953

Implement block on connect with Rust+Node.js SDK #953

Conversation

markmandel commented Jul 27, 2019 • edited Loading

Rust

Node.js

Make

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jul 27, 2019

agones-bot commented Jul 27, 2019

markmandel commented Jul 28, 2019

markmandel commented Jul 28, 2019

markmandel commented Jul 28, 2019

markmandel commented Jul 28, 2019

markmandel commented Jul 28, 2019

agones-bot commented Jul 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jul 29, 2019

agones-bot commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-supersolid commented Jul 29, 2019

markmandel commented Jul 30, 2019

agones-bot commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jul 30, 2019

agones-bot commented Jul 30, 2019

roberthbailey commented Jul 30, 2019

markmandel commented Jul 27, 2019 •

edited

Loading