Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Backward Compatibility of Stream Plan #43

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

RFC: Backward Compatibility of Stream Plan #43

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8ecade
add rfc
st1page Jan 31, 2023
548cc72
rename pr id
st1page Jan 31, 2023
cc5dbd4
fix typo
st1page Jan 31, 2023
b0bc692
Update 0043-backward-compatibility-of-stream-plan.md (#47)
xxchan Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions rfcs/0043-backward-compatibility-of-stream-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
---
feature: Backward Compatibility of Stream Plan
authors:
- "st1page"
start_date: "2023/1/31"
---

# Backward Compatibility of Stream Plan

## Summary

- distinguish the nightly and stable for SQL features and stream plan node protobuf.
- use a Copy-on-Write style on changing the stable stream plan node protobuf.

## Motivation

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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's on earth the requirements we need to meet for rolling-update? 🥵 cc @hzxa21 @arkbriar for a comment.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@xxchan xxchan Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


In conclusion, we need find a way to achieve a balance between rapid development and backward compatibility, especially for stream plan node.

## 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.
Copy link
Member

@fuyufjh fuyufjh Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Nightly and Stable Stream Plan Node
How to know if a SQL Feature has been stable? Developer should comment the compatibility annotation on protobuf struct of every stream plan node(like annotation in java). the annotation contains: "nightly v0.1.14", "stable v0.1.15", "deprecated v0.1.16". A plan node will be with a nightly annotation firstly. When developer ensure that the plan node struct is stable enough, a stable annotation should be comments on the protobuf struct. When developer ensure that frontend will not generate the plan node, a deprecated annotation should be comments on the protobuf struct. A SQL feature is stable means that all the stream plan nodes generated by any version's optimizer should have been stable.

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
Copy link
Member

@xxchan xxchan Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically you mean immutable and versioned protobuf messages? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW we can assume all fields to be required afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. And I think our current stream execution expect that in fact(so many unwrap in from_proto).

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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the any old meta store can store a stream plan with StreamHashAggV1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm in this case, will add a version field in StreamHashAgg a valid choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a special case that we just add a new field and I am not sure if it is general enough


Why make it so complicated and why not just rely on the protobuf's compatibility? To achieve the compatibility, protobuf actually give the struct that all fields are optional. When a protobuf struct is used as a RPC interface, the caller will give a combination of those optional fields and the callee should try best to try all kinds of meaningful combinations or return an error. based on the following facts I think the Copy-on-Write Style Changes is better.
- the changes of the stable plan node is limited. we can make breaking changes in the same release arbitrarily and a stable plan node will not be modified too much. So the duplicated plan node definition will not be too much.
- here the “return error” is unacceptable for us because if we can not resolve the stored streaming plan, the cluster can not boot up anyway. So we must make sure that the compute node can accept any combination of the fields in historical versions. Store all these combination in different version's plannode definition will help to maintain the compatibility, or it will just exist in the compute node's code and easily be forgotten.