Skip to content
This repository has been archived by the owner on Sep 3, 2021. It is now read-only.

we should formalize how we are doing RPC and ensure it meets our current needs #119

Open
phritz opened this issue Aug 26, 2020 · 7 comments

Comments

@phritz
Copy link
Contributor

phritz commented Aug 26, 2020

fixing rocicorp/diff-server#54 without versioning (eg rocicorp/diff-server#55) is a breaking change to existing clients. we should solve this issue the same way for all our rpcs, including between cli and the account service. note there we have slightly more structure, namely a wrapper: https://github.com/rocicorp/account-service/blob/e07de54c1cf601ecb01b743ddd8078061333821d/rpc/wrapper.go#L34.

aaron and i have talked in the past about solving this problem eg with protos and agreed to wait until the need is more pressing. filing this now as that time is approaching rapidly.

notes:

  • for right now to fix the proximate issue the proposal is to introduce a one-off version field into the pullrequest.
  • the diff-server gets pushed from head of master, but even if it didn't we would still need versioning else our releases would be gated by our slowest-to-update client.
@aboodman
Copy link
Contributor

I agree that adding a version field to the pull request nicely solves this problem. Are you advocating for doing more work right now than just fixing the proximate issue? If so what are you advocating for?

@phritz
Copy link
Contributor Author

phritz commented Aug 26, 2020

Are you advocating for doing more work right now than just fixing the proximate issue?

No. I'm saying "let's solve this problem soon" because we are to the point where we want to be careful about what we break and when. I'm not proposing any particular solution.

@aboodman
Copy link
Contributor

aboodman commented Aug 26, 2020 via email

@aboodman
Copy link
Contributor

aboodman commented Aug 26, 2020 via email

@phritz
Copy link
Contributor Author

phritz commented Sep 11, 2020

Or said another way I'm not clear on what problem you're trying to solve.

The problem is how to change the structure or semantics of protocol messages between repc, the diffserver, and the data layer and maybe bindings/repc without breaking what's currently deployed by us or our customers. It's not a "big" problem in the sense that it is hard, it's just an obvious missing piece like if we didn't have logging.

I agree that deferring solving this and similar problems until we need it makes sense. Looks to me like the first customer is getting close to production so seems like we might want to do this soon. Put it this way: by the time we have a customer deploying an ostensibly stable rather than experimental version of their app to their own customers I would like to know how to make a protocol change so that I don't break their app.

Examples of ~recent (or desired) protocol changes that are the kind of thing i'm talking about, think about making these changes with a customer who has deployed to production and has a real userbase:

  1. We want to split the single "clientID" we currently send to diffserver and data layer into separate identifiers specific to the diffserver and data layers. Basically: we want to stop sending clientID and start sending cacheID and branchID.
  2. We started returning a stringified instead of json patch value in the pullresponse to clients that asked for it (by specifying a version)
  3. We will want to start sending a binary patch in the pullresponse to those clients that ask for it.
  4. Instead of an unstructured error string we want to return structured errors from repc to the bindings
  5. We will probably want a way to ask for and receive progressive or prioritized data from the client view in batches. This requires a change to the clientview, clientviewrequest, pullrequest, and possibly pullresponse. The change might be significant enough that rather than stick it into the existing structs we want to send a new struct.
  6. Various small changes like we want to deprecate CommitTransactionResponse.retry_commit and I added head_name to GetRootRequest

These examples boil down to something like wanting to do the following things in as forwards and backwards compatible a fashion as possible:

  • introduce new fields
  • deprecate and ideally remove old fields
  • signal errors in a structured fashion
  • enable callers to ask for specific features or behavior
  • replace a message

We can all imagine a combination of straightforward policies ('ignore all fields you dont understand', 'only add fields -- never change the semantics of a field', etc) and mechanisms ('replies should have structured error field that looks like X', etc.) that solve our current needs. I am suggesting that soon we should say what they are, hopefully something very close to what we are already doing.

Some things we should consider as part of this:

  • it would be nice if we had exactly one way to exchange protocol messages so we could have one policy or set of conventions that works the same way everywhere. Right now we have at lest three ways messages are exchanged:
    • plain json request/response between repc and the data layer and diffserver, and between diffserver and data layer, with errors signaled via HTTP status codes
    • custom rpc-structured json between account cli and account service with structured errors signaled in the response wrapper
    • plain json request/response between bindings and repc with errors signaled through an unstructured string
  • in the repc/diffserver/datalayer case we smear part of the message into the HTTP request, eg X-Replicache-SyncID and Authorization (though Authorization probably makes sense to use with the data layer). We should probably have the policy of just putting everything in the message.
  • we should probably have a first-class way to signal to the sender that they are sending us something we don't understand or support so that old callers throw a comprehensible error message
  • diffserver and half of the data layer (clientview) messages are described in the diffserver repo and consumed by repc, the diffserver, and customer's data layers. the other half of the data layer (batchpush) is described in the repc repo and consumed by repc and the customer's data layer. dispatch interface is described in the rep repo and consumed in the js repo. Is this all exactly as we want it to be?

@phritz phritz changed the title we need to version our rpcs we should formalize how we are doing RPC and ensure it meets our current needs Sep 11, 2020
@arv
Copy link
Contributor

arv commented Sep 14, 2020

I don't think we need to formalize the protocol between repc and replicache-sdk-js. The SDK decides which version it wants to use. It is never intended to use a repc version other than what we get with get-deps.sh

@aboodman
Copy link
Contributor

Good point.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants