Refactored connection configuration & added alpha locator flow #646

jessicafalk · 2018-12-06T11:22:16Z

Description

Added the new alpha locator flow and refactored the Connection config as part of it.
Read in commit order:

Simplifying configurations necessary for connections. The validation we have been doing was very basic (does that string contain anything), so removed them
Updated Worker & WorkerConnectors and added new alpha locator flow
removing tests

When is which flow chosen for the default worker connector?

If we have a Login Token & a PIT -> Alpha Locator
If we have Steam Credentials or only a Login Token -> Locator
Else -> Receptionist

Tests

ran a cloud deployment, connected successfully

Documentation

none yet

Primary reviewers

@jamiebrynes7 @samiwh

workers/unity/Packages/com.improbable.gdk.core/Config/RuntimeConfig.cs

workers/unity/Packages/com.improbable.gdk.core/Worker/DefaultWorkerConnector.cs

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs

workers/unity/Packages/com.improbable.gdk.mobile/Worker/MobileWorkerConnector.cs

ghost · 2018-12-11T12:14:01Z

workers/unity/Packages/com.improbable.gdk.core/Worker/DefaultWorkerConnector.cs

-        /// </summary>
-        /// <returns>True, if should connect via the Locator, false otherwise.</returns>
-        protected override bool ShouldUseLocator()
+        protected override ConnectionService GetConnectionService()


Since this is getting more complicated now, why not accept a command line argument for connectionService? Then we assert the parameters using that. The logic within here is too magical at this point.

(Can be done in another PR, shall I make a ticket for it?)

The launcher does not pass that argument in... so we would need to check for both cases:

it is available

it is not available

Oh I see... will the launcher always be using a particular kind of service?

it provides everything to use the alpha locator flow, but you can also use the normal locator flow

discussed offline, looks good

ghost · 2018-12-11T12:15:19Z

workers/unity/Packages/com.improbable.gdk.core/Worker/DefaultWorkerConnector.cs

+        protected override ConnectionParameters GetConnectionParameters(string workerType, ConnectionService service)
+        {
+            // UseExternalIp needs to be true when using the locator
+            var useExternalIp = service != ConnectionService.Receptionist || UseExternalIp;


This should be service == this || service == that instead of service != other /* this imples it's this or that */ (if we add a new method it'll be a bug)

good point!

ghost · 2018-12-11T12:17:47Z

workers/unity/Packages/com.improbable.gdk.core/Worker/DefaultWorkerConnector.cs

+                        DeploymentTag = steamDeploymentTag,
+                        Ticket = steamTicket,
+                    },
+                    ProjectName = CommandLineUtility.GetCommandLineValue(


What happens if project name is empty?

ghost · 2018-12-11T12:18:55Z

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs

+        /// <param name="connectionFuture">The <see cref="Future{T}"/> of the <see cref="Connection"/> object that we use to connect to the SpatialOS Runtime.</param>
+        /// <param name="workerType">The type of the worker.</param>
+        /// <param name="logger">The logger used by the worker.</param>
+        /// <param name="origin">The position of the worker.</param>


Why was the text changed from "the origin of this worker in local Unity space" changed to "The position of the worker"? The latter is more ambiguous

good point, will change back

samiwh · 2018-12-13T10:54:54Z

workers/unity/Packages/com.improbable.gdk.core/Config/RuntimeConfig.cs

+    /// <summary>
+    ///     Stores the configuration needed to connect via the Alpha Locator.
+    /// </summary>
+    public struct AlphaLocatorConfig


Possibly better to swap these names around. If we know this one is definitely going to be the one we use going forward it might be better to call this the LocatorConfig and the other one the LegacyLocatorConfig.

Or we change the names to say what kind of tokens they are using. We can change this later though so it's not that big a deal.

Main reason why I call them that way is because the Worker SDK is calling them like that. The current locator flow is still the standard locator, while the new locator is in the alpha namespace

samiwh · 2018-12-13T12:00:50Z

workers/unity/Packages/com.improbable.gdk.mobile/Worker/MobileWorkerConnector.cs

+                    UseExternalIp = true,
+                },
+                EnableProtocolLoggingAtStartup = false,
+                DefaultComponentVtable = new ComponentVtable(),


This feels weird to be in the worker connector classes. I think it needs a larger refactor to get it out so there is no change to make for now, but it does feel weird.

I don't find it too weird, as the worker connector takes care of specifying the exact connection logic, but I agree that we could probably separate it out a bit more. Gonna create a ticket to redesign this in the near future

samiwh · 2018-12-13T12:41:08Z

workers/unity/Packages/com.improbable.gdk.core/Worker/Worker.cs

+        /// <param name="logger">The logger used by this worker.</param>
+        /// <param name="origin">The origin of this worker in the local Unity space.</param>
+        /// <returns>A <see cref="Task{TResult}"/> to run this method asyncally and retrieve the created <see cref="Worker"/> object upon connecting successfully.</returns>
+        private static async Task<Worker> TryToConnect(Future<Connection> connectionFuture,


Nit - All async functions should have the world Async in them. So this should be TryToConnectAsync.

* Cleanup pass * Remove include

improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 150-299 lines, ignoring generated files. labels Dec 6, 2018

jessicafalk force-pushed the feature/pit branch from 41363d7 to 602e797 Compare December 6, 2018 12:39

improbable-prow-robot added size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 150-299 lines, ignoring generated files. labels Dec 7, 2018

jessicafalk force-pushed the feature/pit branch from 63b1e28 to 31e706c Compare December 7, 2018 15:37

jessicafalk changed the title ~~[WIP] new locator flow~~ added alpha locator flow Dec 7, 2018

improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2018

jessicafalk requested review from jamiebrynes7, samiwh and zeroZshadow and removed request for jamiebrynes7 December 7, 2018 15:40

jessicafalk changed the title ~~added alpha locator flow~~ Refactored connection configuration & added alpha locator flow Dec 7, 2018

jamiebrynes7 reviewed Dec 7, 2018

View reviewed changes

ghost reviewed Dec 11, 2018

View reviewed changes

jessicafalk force-pushed the feature/pit branch from 140cd83 to 5cd27e2 Compare December 11, 2018 14:50

ghost approved these changes Dec 11, 2018

View reviewed changes

jamiebrynes7 approved these changes Dec 12, 2018

View reviewed changes

samiwh reviewed Dec 13, 2018

View reviewed changes

jessicafalk added 8 commits December 14, 2018 10:40

simplifying configs

5413f7e

adding new locator flow

8a4e38d

deleting tests

f13e9b9

Docs

944f7fd

PR feedback

99122e9

fixing merge conflicts

939e0b3

fixing locator part

5592cc4

changing method name

f9e28a1

jessicafalk force-pushed the feature/pit branch from cf8d530 to f9e28a1 Compare December 14, 2018 10:41

added release notes

d659d82

samiwh approved these changes Dec 14, 2018

View reviewed changes

jessicafalk merged commit 624dd24 into master Dec 14, 2018

jessicafalk deleted the feature/pit branch December 14, 2018 15:04

jessicafalk pushed a commit that referenced this pull request Nov 15, 2019

Add FUnrealObjRef.IsValid() (#646)

1d36ed8

* Cleanup pass * Remove include

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored connection configuration & added alpha locator flow #646

Refactored connection configuration & added alpha locator flow #646

jessicafalk commented Dec 6, 2018 •

edited

Loading

ghost Dec 11, 2018

jessicafalk Dec 11, 2018

ghost Dec 11, 2018

jessicafalk Dec 11, 2018

ghost Dec 11, 2018

ghost Dec 11, 2018

jessicafalk Dec 11, 2018

ghost Dec 11, 2018

ghost Dec 11, 2018

jessicafalk Dec 11, 2018

samiwh Dec 13, 2018

samiwh Dec 13, 2018

jessicafalk Dec 13, 2018

samiwh Dec 13, 2018

jessicafalk Dec 13, 2018

samiwh Dec 13, 2018 •

edited

Loading

jessicafalk Dec 13, 2018

Refactored connection configuration & added alpha locator flow #646

Refactored connection configuration & added alpha locator flow #646

Conversation

jessicafalk commented Dec 6, 2018 • edited Loading

Description

Tests

Documentation

Primary reviewers

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

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

samiwh Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicafalk commented Dec 6, 2018 •

edited

Loading

samiwh Dec 13, 2018 •

edited

Loading