030 RC1 issue thread updates googleforgames#55

CHANGELOG.md

@@ -6,7 +6,7 @@
 ### Release notes
 - The Frontend API calls have all be changed to reflect the fact that they operate on Players in state storage.  To queue a game client, 'CreatePlayer' in Open Match, to get updates 'GetUpdates', and to stop matching, 'DeletePlayer'. The calls are now much more obviously related to how Open Match sees players: they are database records that it creates on demand, updates using MMFs and the Backend API, and deletes when the player is no longer looking for a match.  
 - The Player record in state storage has changed to a more complete hash format, and it no longer makes sense to remove a player's assignment from the Frontend as a separate action to removing their record entirely.  `DeleteAssignment()` has therefore been removed.  Just use `DeletePlayer` instead; you'll always want the client to re-request matching with its latest attributes anyway.
-- There is now a module for [indexing and deindexing players in state storage](internal/statestorage/redis/playerindices/playerindices.go).  This is a *much* more effecient, as well as being cleaner and more maintainable than the previous implementation which was **hard-coded to index everything** you passed in to the Frontend API at a specific JSON object depth.
+- There is now a module for [indexing and deindexing players in state storage](internal/statestorage/redis/playerindices/playerindices.go).  This is a *much* more efficient, as well as being cleaner and more maintainable than the previous implementation which was **hard-coded to index everything** you passed in to the Frontend API at a specific JSON object depth.
     - This paves the way for dynamically choosing your indicies without restarting the matchmaker.  This will be implemented if there is demand.  Pull Requests are welcome!
     - Two internal timestamp-based indices have replaced the previous `timestamp` index. `created` is used to calculate how long a player has been waiting for a match, `accessed` is used to determine when a player needs to be expired out of state storage.  Both are prefixed by the string `OM_METADATA` so it should be easy to spot them.
 - A call to the Frontend API `GetUpdates()` gRPC endpoint returns a stream of player messages. This is used to send updates to state storage for the `Assignment`, `Status`, and `Error` Player fields in near-realtime. **It is the responsibility of the game client to disconnect** from the stream when it has gotten the results it was waiting for!

api/protobuf-spec/messages.proto

@@ -73,8 +73,8 @@ message Player{
   string pool = 3;                  // Optionally used to specify the PlayerPool in which to find a player. 
   repeated Attribute attributes= 4; // Attributes of this player.
   string assignment = 5;            // By convention, ip:port of a DGS to connect to 
-  string status = 6;                // 
-  string error = 7;                 // 
+  string status = 6;                // Arbitrary developer-chosen string. 
+  string error = 7;                 // Arbitrary developer-chosen string.
 }
 
 

cmd/backendapi/apisrv/apisrv.go

@@ -107,7 +107,7 @@ func (s *BackendAPI) Open() error {
 }
 
 // CreateMatch is this service's implementation of the CreateMatch gRPC method
-// defined in ../proto/backend.proto
+// defined in api/protobuf-spec/backend.proto
 func (s *backendAPI) CreateMatch(c context.Context, profile *backend.MatchObject) (*backend.MatchObject, error) {
 
 	// Get a cancel-able context
@@ -309,7 +309,7 @@ func (s *backendAPI) ListMatches(p *backend.MatchObject, matchStream backend.Bac
 }
 
 // DeleteMatch is this service's implementation of the DeleteMatch gRPC method
-// defined in ../proto/backend.proto
+// defined in api/protobuf-spec/backend.proto
 func (s *backendAPI) DeleteMatch(ctx context.Context, mo *backend.MatchObject) (*backend.Result, error) {
 
 	// Create context for tagging OpenCensus metrics.
@@ -341,7 +341,7 @@ func (s *backendAPI) DeleteMatch(ctx context.Context, mo *backend.MatchObject) (
 }
 
 // CreateAssignments is this service's implementation of the CreateAssignments gRPC method
-// defined in ../proto/backend.proto
+// defined in api/protobuf-spec/backend.proto
 func (s *backendAPI) CreateAssignments(ctx context.Context, a *backend.Assignments) (*backend.Result, error) {
 
 	// Make a map of players and what assignments we want to send them.
@@ -402,7 +402,7 @@ func (s *backendAPI) CreateAssignments(ctx context.Context, a *backend.Assignmen
 }
 
 // DeleteAssignments is this service's implementation of the DeleteAssignments gRPC method
-// defined in ../proto/backend.proto
+// defined in api/protobuf-spec/backend.proto
 func (s *backendAPI) DeleteAssignments(ctx context.Context, r *backend.Roster) (*backend.Result, error) {
 	assignments := getPlayerIdsFromRoster(r)
 
@@ -415,20 +415,6 @@ func (s *backendAPI) DeleteAssignments(ctx context.Context, r *backend.Roster) (
 		"numAssignments": len(assignments),
 	}).Info("gRPC call executing")
 
-	/*
-		// TODO: relocate this redis functionality to a module
-		redisConn := s.pool.Get()
-		defer redisConn.Close()
-
-		// Remove player assignments in a transaction
-		redisConn.Send("MULTI")
-		// TODO: make playerIDs a repeated protobuf message field and iterate over it
-		for _, playerID := range assignments {
-			beLog.WithFields(log.Fields{"query": "DEL", "key": playerID}).Debug("state storage operation")
-			redisConn.Send("DEL", playerID)
-		}
-		_, err := redisConn.Do("EXEC")
-	*/
 	err := redisHelpers.DeleteMultiFields(ctx, s.pool, assignments, "assignment")
 
 	// Issue encountered
@@ -449,6 +435,8 @@ func (s *backendAPI) DeleteAssignments(ctx context.Context, r *backend.Roster) (
 	return &backend.Result{Success: true, Error: ""}, err
 }
 
+// getPlayerIdsFromRoster returns the slice of player ID strings contained in
+// the input roster.
 func getPlayerIdsFromRoster(r *backend.Roster) []string {
 	playerIDs := make([]string, 0)
 	for _, p := range r.Players {

cmd/frontendapi/apisrv/apisrv.go

@@ -217,6 +217,9 @@ func (s *frontendAPI) GetUpdates(p *frontend.Player, assignmentStream frontend.F
 			errTag, _ := tag.NewKey("errtype")
 			fnCtx, _ := tag.New(ctx, tag.Insert(errTag, "watch_timeout"))
 			stats.Record(fnCtx, FeGrpcErrors.M(1))
+			//TODO: we could generate a frontend.player message with an error
+			//field and stream it to the client before throwing the error here
+			//if we wanted to send more useful client retry information
 			return err
 
 		case a := <-watchChan:

internal/statestorage/redis/playerindices/playerindices.go

@@ -151,7 +151,7 @@ func Create(ctx context.Context, rPool *redis.Pool, cfg *viper.Viper, player om_
 				iLog.WithFields(log.Fields{"attribute": attribute, "value": v.Raw}).Debug("Couldn't find index in JSON: ", player.Properties)
 				continue
 			} else if -9223372036854775808 <= v.Int() && v.Int() <= 9223372036854775807 {
-				// value contains a valid unsigned 64-bit integer
+				// value contains a valid 64-bit integer
 				value = v.Int()
 			} else {
 				iLog.WithFields(log.Fields{"attribute": attribute}).Debug("No valid value for attribute, not indexing")

internal/statestorage/redis/redispb/player.go

@@ -128,7 +128,7 @@ func PlayerWatcher(ctx context.Context, pool *redis.Pool, pb om_messages.Player)
 				if err != nil {
 					// Not fatal, but this error should be addressed.  This could
 					// cause the player to expire while still actively connected!
-					pwLog.Error("State storage error:", err.Error())
+					pwLog.WithFields(log.Fields{"error": err.Error()}).Error("Unable to update accessed metadata timestamp")
 				}
 
 				// Get player from redis.
@@ -153,7 +153,9 @@ func PlayerWatcher(ctx context.Context, pool *redis.Pool, pb om_messages.Player)
 				// This can be made much cleaner if protobuffer reflection improves.
 				curResults := fmt.Sprintf("%v%v%v", results.Assignment, results.Status, results.Error)
 				if prevResults == curResults {
-					pwLog.Debug("No new results, backing off")
+					pwLog.Debug("No new watcher results")
+					// TODO: change the debug message once exp bo + jitter is implemented
+					//pwLog.Debug("No new results, backing off")
 					time.Sleep(2 * time.Second) // TODO: exp bo + jitter
 				} else {
 					// Return value retreived from Redis