SDKServer Player Tracking implementation #1507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markmandel
added
kind/feature
markmandel
requested review from
aLekSer and
pooneh-m
and removed request for
EricFortin
|
Build Succeeded 👏 Build Id: 059fb6fa-d728-4f14-b1c7-903a3dc95ada The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver_test.go
Outdated
| assert.False(t, ok.Bool, "no player connected yet") | ||
|
|
||
| // sdk value should always be correct, even if we send more than one update per second. | ||
| for i := 0; i < 3; i++ { |
There was a problem hiding this comment.
Probably it would be easier to understand why 3 (if I got it right):
Suggested change
| for i := 0; i < 3; i++ { | |
| for i := 0; i < capacity; i++ { |
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver_test.go
Outdated
| } | ||
| count, err = sc.GetPlayerCount(context.Background(), e) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, int64(3), count.Count) |
There was a problem hiding this comment.
Same as above?
Suggested change
| assert.Equal(t, int64(3), count.Count) | |
| assert.Equal(t, capacity, count.Count) |
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver_test.go
Outdated
| // on an update, confirm that the update hits the K8s api, only once | ||
| select { | ||
| case value := <-updated: | ||
| assert.Equal(t, int64(3), value.Count) |
There was a problem hiding this comment.
here also
Suggested change
| assert.Equal(t, int64(3), value.Count) | |
| assert.Equal(t, capacity, value.Count) |
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver_test.go
Outdated
|
|
||
| // sdk value should always be correct, even if we send more than one update per second. | ||
| // let's leave one player behind | ||
| for i := 0; i < 2; i++ { |
There was a problem hiding this comment.
Suggested change
| for i := 0; i < 2; i++ { | |
| for i := 0; i < capacity-1; i++ { |
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver_test.go
Outdated
| id := &alpha.PlayerID{PlayerID: token} | ||
| ok, err := sc.PlayerDisconnect(context.Background(), id) | ||
| assert.NoError(t, err) | ||
| assert.True(t, ok.Bool, "Player "+token+" should be disconnected") |
There was a problem hiding this comment.
Might be a bit easier to read
Suggested change
| assert.True(t, ok.Bool, "Player "+token+" should be disconnected") | |
| assert.True(t, ok.Bool, "Player %s should be disconnected", token) |
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver.go
Outdated
| @@ -547,35 +555,108 @@ func (s *SDKServer) stopReserveTimer() { | |||
| // [Stage:Alpha] | |||
| // [FeatureFlag:PlayerTracking] | |||
| func (s *SDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { | |||
| panic("implement me") | |||
| if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { | |||
| return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled") | |||
There was a problem hiding this comment.
Could be something like:
Suggested change
| return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled") | |
| return nil, errors.New(fmt.Sprintf("feature gate '%s' is not enabled", runtime.FeaturePlayerTracking)) |
There was a problem hiding this comment.
We've currently got this code all through localsdk.go as well. Should I change both?
There was a problem hiding this comment.
In fact, this should errors.Errorf(...) - which is much better. I'll make the change here, and come back around to localsdk.go in a later PR.
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver.go
Outdated
| } | ||
|
|
||
| if int64(len(s.gsConnectedPlayers)) >= s.gsPlayerCapacity { | ||
| return &alpha.Bool{}, errors.New("Players are already at capacity") |
There was a problem hiding this comment.
lower case for errors
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
Suggested change
| return &alpha.Bool{}, errors.New("Players are already at capacity") | |
| return &alpha.Bool{}, errors.New("players are already at capacity") |
aLekSer
reviewed
May 1, 2020
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver.go
Outdated
| if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { | ||
| return nil, errors.New(string(runtime.FeaturePlayerTracking) + " not enabled") | ||
| } | ||
| s.logger.WithField("PlayerId", id).Debug("Player Disconnected") |
There was a problem hiding this comment.
Not sure if previous version parse pointers in a correct way.
Suggested change
| s.logger.WithField("PlayerId", id).Debug("Player Disconnected") | |
| s.logger.WithField("PlayerId", id.PlayerID).Debug("Player Disconnected") |
aLekSer
reviewed
May 1, 2020
aLekSer
reviewed
May 1, 2020
pkg/sdkserver/sdkserver.go
Outdated
| if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { | ||
| return errors.New(string(runtime.FeaturePlayerTracking) + " not enabled") | ||
| } | ||
| s.logger.WithField("playerIDs", s.gsConnectedPlayers).Debug("updating connected players") |
There was a problem hiding this comment.
Might go also after lock.
Suggested change
| s.logger.WithField("playerIDs", s.gsConnectedPlayers).Debug("updating connected players") | |
| s.gsUpdateMutex.RLock() | |
| s.logger.WithField("playerIDs", s.gsConnectedPlayers).Debug("updating connected players") |
aLekSer
reviewed
May 1, 2020
markmandel
force-pushed
the
feature/sdk-server-tracking
branch
from
35c17f9
to
79dc130
Compare
|
Build Succeeded 👏 Build Id: cbd43784-331a-42c9-8f63-96ab7698143b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
aLekSer
reviewed
May 4, 2020
pkg/sdkserver/sdkserver.go
Outdated
| if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { | ||
| return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) | ||
| } | ||
| s.logger.WithField("PlayerId", id).Debug("Player Connected") |
There was a problem hiding this comment.
Sorry Mark, one more
Suggested change
| s.logger.WithField("PlayerId", id).Debug("Player Connected") | |
| s.logger.WithField("PlayerId", id.PlayerID).Debug("Player Connected") |
There was a problem hiding this comment.
Good catch. Applying now!
aLekSer
approved these changes
May 4, 2020
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aLekSer, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Implementation of the SDK gRPC Server, with accompanying unit tests. Work on googleforgames#1033
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 6, 2020
Cleanup the remainder
`errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")`
to be being
`errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking)`,
which is much nicer.
Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 6, 2020
Includes implementations and unit tests for PlayerConnect, PlayerDisconnect, GetPlayerCount, IsPlayerConnected and GetConnectedPlayers. Conformance tests will come in the next PR. Work on googleforgames#1507
markmandel
added a commit
that referenced
this pull request
May 6, 2020
Cleanup the remainder
`errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")`
to be being
`errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking)`,
which is much nicer.
Work on #1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 6, 2020
Includes implementations and unit tests for PlayerConnect, PlayerDisconnect, GetPlayerCount, IsPlayerConnected and GetConnectedPlayers. Conformance tests will come in the next PR. Work on googleforgames#1507
markmandel
added a commit
that referenced
this pull request
May 7, 2020
Includes implementations and unit tests for PlayerConnect, PlayerDisconnect, GetPlayerCount, IsPlayerConnected and GetConnectedPlayers. Conformance tests will come in the next PR. Work on #1507 Co-authored-by: Alexander Apalikov <alexander.apalikov@globant.com>
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 7, 2020
Conformance tests for the Go SDK. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 7, 2020
Conformance tests for the Go SDK. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 7, 2020
Conformance tests for the Go SDK. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 10, 2020
Conformance tests for the Go SDK. Work on googleforgames#1507
roberthbailey
pushed a commit
that referenced
this pull request
May 10, 2020
Conformance tests for the Go SDK. Work on #1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 11, 2020
Conformance tests for the REST API. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 12, 2020
Conformance tests for the REST API. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 12, 2020
Conformance tests for the REST API. Work on googleforgames#1507
roberthbailey
pushed a commit
that referenced
this pull request
May 12, 2020
Conformance tests for the REST API. Work on #1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 13, 2020
Updated the udp-simple example to accept various player tracking commands, and also implemented e2e tests to test it working on a singular GameServer instance. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 13, 2020
Updated the udp-simple example to accept various player tracking commands, and also implemented e2e tests to test it working on a singular GameServer instance. Work on googleforgames#1507
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
May 13, 2020
Updated the udp-simple example to accept various player tracking commands, and also implemented e2e tests to test it working on a singular GameServer instance. Work on googleforgames#1507
markmandel
added a commit
that referenced
this pull request
May 14, 2020
* E2E Tests for GameServer Player Tracking Updated the udp-simple example to accept various player tracking commands, and also implemented e2e tests to test it working on a singular GameServer instance. Work on #1507
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
Implementation of the SDK gRPC Server, with accompanying unit tests. Work on googleforgames#1033
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
Cleanup the remainder
`errors.New(string(runtime.FeaturePlayerTracking) + " not enabled")`
to be being
`errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking)`,
which is much nicer.
Work on googleforgames#1507
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
) Includes implementations and unit tests for PlayerConnect, PlayerDisconnect, GetPlayerCount, IsPlayerConnected and GetConnectedPlayers. Conformance tests will come in the next PR. Work on googleforgames#1507 Co-authored-by: Alexander Apalikov <alexander.apalikov@globant.com>
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
Conformance tests for the Go SDK. Work on googleforgames#1507
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
Conformance tests for the REST API. Work on googleforgames#1507
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
* E2E Tests for GameServer Player Tracking Updated the udp-simple example to accept various player tracking commands, and also implemented e2e tests to test it working on a singular GameServer instance. Work on googleforgames#1507
ilkercelikyilmaz
pushed a commit
to ilkercelikyilmaz/agones
that referenced
this pull request
Oct 23, 2020
* E2E Tests for GameServer Player Tracking Updated the udp-simple example to accept various player tracking commands, and also implemented e2e tests to test it working on a singular GameServer instance. Work on googleforgames#1507
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Implementation of the SDK gRPC Server, with accompanying unit tests.
Which issue(s) this PR fixes:
Work on #1033
Special notes for your reviewer:
None.