RFC: Backward Compatibility of Stream Plan #43
Conversation
| To be discussed: what is the proper format of those comments in proto files and how to check all plan node should have one in CI check? | ||
|
|
||
| ### Copy-on-Write Style Changes on Stable Plan Node Protobuf | ||
| How to maintain the compatibility of the plan node's protobuf? If developer want to do any changes on a stable plan node, he should add a new plan node protobuf definition. For example, if he want to add a new field in `StreamHashAgg`, he must define a new protobuf struct `StreamHashAggV2` and add the field on that. Notice that there are multi versions protobuf but they can share the same implementation. |
There was a problem hiding this comment.
Should we keep all StreamHashAgg versions even after the release?
If we have two versions of StreamHashAggV2 in a release, will CN also handle V1?
There was a problem hiding this comment.
Yes, because the any old meta store can store a stream plan with StreamHashAggV1
There was a problem hiding this comment.
hmm in this case, will add a version field in StreamHashAgg a valid choice?
There was a problem hiding this comment.
it is a special case that we just add a new field and I am not sure if it is general enough
|
|
||
| In https://github.com/risingwavelabs/rfcs/issues/41, we discuss the backward compatibility. And the protobuf structure of stream plan nodes is a special part. | ||
| - the plan node's structure usually modified more frequently than other protobuf structure such as catalog, especially when we are developing new SQL features and we even do not know how to do it right. The plan node's changes are not only adding some optional field(which can be solved by protobuf) but also of meaning and behaviors of the operator. For example, our state table information of streamAgg having breaking changed in 0.1.13 and in 0.1.16, the source executor is no longer responsible for generating row_id. And we do not confirm the sort and overAgg's format so far. | ||
| - in other databases, the plan node is just used as a communicating protocol between frontend and compute node. So the compute node can only support the latest version's plan node format and reject all the requests with unknown plan node. But our stream plan should be persistent in meta store which means that a compute node must be compatible with all versions of old plans' protobuf format. |
There was a problem hiding this comment.
But our stream plan should be persistent in meta store which means that a compute node must be compatible with all versions of old plans' protobuf format.
I'm thinking if it is reasonable to force users rebuild the plan when incompatible🤣 Making CN compatible with all versions can be challenging.
There was a problem hiding this comment.
that's what I want to do on MV with the nightly SQL features. But it is not good enough. consider some data have been consumed and despaired in the source. how to reuse these state data,
There was a problem hiding this comment.
in other databases, the plan node is just used as a communicating protocol between frontend and compute node. So the compute node can only support the latest version's plan node format and reject all the requests with unknown plan node
Our batch can follow the same approach? Maybe we can discuss this issue together in this RFC for completeness and comparison 🤔
There was a problem hiding this comment.
If the consideration is only for rolling-update. And we only want to reject old requests, does it mean that we don't even need protobuf compatibility for communication? (Just let gRPC report error for the client...)
One problem is the request from new client to old server. But we can avoid it by updating servers first.
There was a problem hiding this comment.
In general, the rolling upgrade requires that the cluster finally works and breaks nothing (temporarily unavailable is fine for some services). Ideally, we should consider each version combination to prevent unexpected behaviors. But that would be too much. We could add constraints like the compute must be upgraded after the meta, proposed by @hzxa21 in another discussion. We can add these constraints by either updating the deployment tools (enforce the order) or the kernel (deny of service when mismatch). If upgrading compute nodes requires rebuilding the streaming plan and its states, we should ensure that's worth it and try to automate it, e.g., fixing a buggy operator and the states as well. Otherwise, we should keep it compatible.
There was a problem hiding this comment.
I can understand the spirit "breaks nothing (but temporarily unavailable is fine)", but it's still a little big vague to me.
To be more specific, it is understandable that things need to be persisted (like stream plan) should be taken care of. But what confused me is that is it ok to break all other things, i.e., the communication protocol? 🤔 IMHO that would just make the service temporarily unavailable, and will come back to normal after upgrade finishes. Correct me if I'm wrong.
There was a problem hiding this comment.
What's on earth the requirements we need to meet for rolling-update? 🥵 cc @hzxa21 @arkbriar for a comment.
IMO, in production, the requirements are:
- Be able to rollback: we must provide a mechnism to rollback the upgrade in case there is any performance/corrcetness issue observered by user during rolling upgrade. Note that upgrading to a new version doesn't mean the users have adopted the new features brought by this version. We normally don't need to ensure the ability to rollback after users have adopted the new features because it is hard and not realistic.
- Minimize downtime: ideally user wants zero performance/correctness impacts during (and after) upgrade. There are normally two approaches to minimize the downtime:
a. Ensure two versions of codes are compatible during the upgrade period so the number of out-of-service nodes are controllable.
b. Speed up the upgrade and the warm-up/recovery period after the upgrade.
There was a problem hiding this comment.
I can understand the spirit "breaks nothing (but temporarily unavailable is fine)", but it's still a little big vague to me.
To be more specific, it is understandable that things need to be persisted (like stream plan) should be taken care of. But what confused me is that is it ok to break all other things, i.e., the communication protocol? 🤔 IMHO that would just make the service temporarily unavailable, and will come back to normal after upgrade finishes. Correct me if I'm wrong.
It all depends on how fast the upgrade is and whether we allow rollback as I mentioned above. Sometimes fast upgrade contradicts with the ability to rollback, especially when breaking changes are introduced. Rolling out breaking changes as fast as possible can minimize unavailbiliy but hurt ability to rollback if we don't maintain compatibility because we won't be able to rollback after upgrade finishes. However, since we are in early production stage and in rapid development, I think it is okay to relax the requirement and introduce breaking changes without considering too much about ability to rollback.
There was a problem hiding this comment.
But what confused me is that is it ok to break all other things, i.e., the communication protocol? 🤔 IMHO that would just make the service temporarily unavailable, and will come back to normal after upgrade finishes.
Yes, it's ok in most cases. If you think of upgrading the simplest service, a stateless web service, there will be a time that users will get errors when the old services are shut down one by one, but it recovers quickly, thanks to the LB. This kind of outage is allowed and accepted when upgrading non-availability-critical applications by most users.
Besides, I totally agree with what @hzxa21 said, the ability to rollback and the downtime matter greatly. At least one of these two should be considered carefully if not both can be satisfied.
| ## Design | ||
|
|
||
| ### Nightly and Stable SQL Features | ||
| Distinguish the nightly and stable feature when publishing release version. RW will do not ensure compatibility for the streaming jobs with the nightly features in following releases. For example, if we release the "emit on close" as a nightly feature in the release v0.1.17 and user create a mv with that feature on a v0.1.17 cluster. The v0.1.18 and following version's RW can not ensure it can run successfully on the existing streaming jobs. User can drop the MVs with the nightly feature before they upgrade the cluster. For those nightly feature users really what to upgrade, we can write helper scripts too. And the stable features will be tested with new released compute node on old version streaming plans. Also, with the convinced stable feature list, we can test the backward compatibility more easily. |
There was a problem hiding this comment.
I prefer the word "experimental" than "nightly" because it's more straightforward to common people.
BTW, when delivering the delta join feature (by Dylan), we had discussed this and decided to mark it as "experimental", especially on the user docs.
|
|
||
| To be discussed: what is the proper format of those comments in proto files and how to check all plan node should have one in CI check? | ||
|
|
||
| ### Copy-on-Write Style Changes on Stable Plan Node Protobuf |
There was a problem hiding this comment.
So basically you mean immutable and versioned protobuf messages? 🤔
There was a problem hiding this comment.
BTW we can assume all fields to be required afterwards?
There was a problem hiding this comment.
Yes, I think so. And I think our current stream execution expect that in fact(so many unwrap in from_proto).
| To be discussed: What is the proper format of these comments in the proto files and how to check that all plan nodes have one in CI checks? | ||
|
|
||
| ### Copy-on-Write Style Changes on Stable Plan Node Protobuf | ||
| How to maintain compatibility of the plan node's protobuf? If a developer wants to make changes to a stable plan node, he should add a new plan node protobuf definition. For example, if they want to add a new field in `StreamHashAgg`, they must define a new protobuf struct `StreamHashAggV2` and add the field to it. Note that there can be multiple versions of protobuf, but they can share the same implementation. |
There was a problem hiding this comment.
If the only change allowed is to add new fields, we can just add a version field in the protobuf message and add new fields to the same message defintion instead of creating multiple protobuf definitions.
There was a problem hiding this comment.
It’s discussed here #43 (comment)
adding field is just one of the cases🤔 Any change is allowed
No description provided.