[Draft]: NEX Service - #5
Conversation
|
The latest Buf updates on your PR. Results from workflow Validate Pull Request / check_buf (pull_request).
|
|
Should we add protocol-specific methods here as well? Such as methods to get DataStore objects, and match session data, and rankings, etc? |
|
I think there should be services for each protocol, we'll probably have various methods on each protocol so I believe it's best to separate them to keep them organized |
| import "nex/v1/kick_user_aggressive_rpc.proto"; | ||
| import "nex/v1/kill_user_connection_rpc.proto"; | ||
|
|
||
| service NEXServiceV1 { |
There was a problem hiding this comment.
It isn't necessary to add the V1 suffix to the service name since that is already handled by the package name nex.v1
There was a problem hiding this comment.
It is, gRPC differentiates services by their name. We just got lucky in the other services that we happened to rename them in their respective v2 updates
There was a problem hiding this comment.
The service name isn't relevant here, so long as the package name is updated. Let's look at an example from the Go account service:
The "v1" version (with package name account) is located on "github.com/PretendoNetwork/grpc/go/account", and the server and client interfaces are prefixed with the service name: AccountClient
The v2 version (with package name account.v2), however, is located on "github.com/PretendoNetwork/grpc/go/account/v2" and the server and client interfaces are prefixed with the new service name: AccountServiceClient
Even if the service names were the same, they are still located at different scopes in code, so there isn't any issues with keeping the name as NEXService across versions, since the code for them would be stored at different locations and therefore they can't collision.
In this case, the package name is the following:
So there won't be any issues with keeping the service name the same as long as the package name is updated to v2 accordingly when needed
There was a problem hiding this comment.
I was partially mistaken. gRPC uses both the service name and package name to create the identifiers (this can be seen in the account server, where our middleware checks path names and these path names include the service identifier. Examples being /api.API/GetUserData and /api.v2.ApiService/GetUserData), so the service name is still relevant, since it means we could keep everything under the same package but with different service names, or we could use different packages (like what we do now) with the same package names
That being said, I still believe it's good to version the service name as well for cases where we need to support multiple versions at the same time. The account server is a good recent example of that, where because we didn't version the service names and the names look so similar (AccountDefinition vs AccountServiceDefinition) it's hard to see at a glance what version each service is, resulting in code like:
import { AccountDefinition as AccountServiceDefinitionV1 } from '@pretendonetwork/grpc/account/account_service';
import { AccountServiceDefinition as AccountServiceDefinitionV2 } from '@pretendonetwork/grpc/account/v2/account_service';But if instead we had versioned the service names, we would not need for a namespace import:
import { AccountServiceDefinitionV1 } from '@pretendonetwork/grpc/account/account_service';
import { AccountServiceDefinitionV2 } from '@pretendonetwork/grpc/account/v2/account_service';
That's fair, I agree. Do we want them in separate services entirely (IE, putting them at the top level of the protobufs Personally I'm leaning towards putting them inside the So we could have it structured like:
|
That sounds good to me! |
There was a problem hiding this comment.
Can we move this into a protobufs/nex/matchmaking/v1 service? That was the agreed upon convention earlier in this PR (#5 (comment))
Also, ideally these methods would map pretty well to the actions/data structures found in the protocols. So for example, instead of trying to define "matches" here we should just add a Gathering and MatchmakeSession structure instead, and have methods like ListGatherings and ListMatchmakeSessions (with flags like "only active" and such)
I don't think we need to completely remake the 3 separate matchmaking protocols, to be clear, but just lining things up as best we can so the data maps more cleanly
@DaniElectra and I can give you any details you may need to help push that forward
There was a problem hiding this comment.
The main reason I had done it this was is because the current implementation I put together is pulling data from both matchmake_sessions and gatherings. I can implement generic methods for the other schemas no problem. But would it not still make sense to have combined methods available? I just worry it would be inefficient to have to make 2 grpc calls on the client side and aggregate the data after the fact when it's just a single sql query that needs to occur
There was a problem hiding this comment.
MatchmakeSession inherits from Gathering, so any data on a Gathering would also be available on a MatchmakeSession. That only requires 1 call, not 2. In NEX, this is how the data types work:
- Gathering is the base type. It effectively just represents "a group of 0 or more participants", where a "participant" can be either a player or something like the server (such as the owner of official tournaments). In RDV this is actually the only type that exists for this, so it wears many hats, but Nintendo sort of redefined this
- MatchmakeSession inherits from Gathering. It has all the same data as one, but represents a joinable active session of connected stations. This is almost always what we work with
- PersistentGathering also inherits from Gathering. It has all the same data as one, but it does not represent an active session. Normally a Gathering is unregistered when it has no more participants, and users need to be physically online to be registered as a participant, but a PersistentGathering can stay registered on the server even with 0 participants, and a user can be offline and still remain a registered participant of the PersistentGathering. This is how things like tournaments work, which are long-standing gatherings that people can join and stay joined into (like joining a team)
So having methods like ListGatherings, ListMatchmakeSessions and ListPersistentGatherings would simply be used for different contexts. But since the latter 2 types inherit from Gathering, you get all the same data from ListGatherings in them as well just as part of the data type
Your "GetActiveMatchs" would effectively just be ListMatchmakeSessions, ListMatchmakeSessions is just a more accurate name and would return the actual MatchmakeSession as a field, and my suggestion to have it take in certain flags like is_active would make the method generic enough to be used in situations like SplatNet which need live data and getting historical data for moderation purposes
The point of these protocols is, at it's core, to basically provided a non-NEX way of interacting with NEX. So that's how it should be designed. Deviations should only occur when absolutely necessary, which isn't the case here
There was a problem hiding this comment.
Also you shouldn't manually rebuild the protos, the bot does that on merge
There was a problem hiding this comment.
I'm aware, I'm just doing it for now since I'm also pulling this in and actively implementing it in both the Splatoon server and the Splatoon website. Long term it would probably make more sense to implement this in the common lib or somewhere similar, but I'm looking to get this off the ground fairly quickly here
There was a problem hiding this comment.
For future reference, you can do that without pushing upstream. You can make a go.work file which can override imports. For example, this is my go.work that I use in my local copy of the common lib to override the protocol lib import to my own local copy:
go 1.24.0
toolchain go1.24.3
use .
replace (
github.com/PretendoNetwork/nex-protocols-go/v2 => ../nex-protocols-go
)That way you can compile the protos locally and just override the import, without needing to commit those changes and push them upstream. The CI should be doing that in all normal cases, and it reduces clutter in the PR (since now this PR lists 44 changed files, most of which are not the actual proto files being changed)
Resolves #XXX
Changes:
This is the start of a generic NEX service. Anything that we may want to do with, or get from, a game server should be implemented here. Currently using a PID size of uint64 because there's no reason not to in this case, it won't break anything and makes it forwards-compatible
Marked as a draft because this is very WIP. I'm not sure what all we think we might need out of this, and the design is subject to change at any time