Harden restatectl reconfigure and avoid sending full LogletParams
#2740
Conversation
|
@tillrohrmann totally not urgent, but this is a follow up to #2730 now it's fully up to the node to figure out the new loglet-id. The reconfigure command only sends hints to the node, including the nodeset and the sequencer id, which are completely optional if previous loglet was also a replicated loglet |
There was a problem hiding this comment.
Thank you for making this better, @muhamadazmy!
Slight concern - I think we should try a bit harder to at least detect cross-version incompatibilities. I know it's annoying since we only just shipped 1.2.0 but now the possibility exists of using mismatched restate-server and restatectl versions together. Even if we don't support backwards compatibility, should we at least detect and fail loudly if we get a request with the old string params field set?
| @@ -90,12 +90,19 @@ message CreatePartitionSnapshotRequest { uint32 partition_id = 1; } | |||
| message CreatePartitionSnapshotResponse { string snapshot_id = 1; } | |||
|
|
|||
| message ChainExtension { | |||
| reserved 5; | |||
There was a problem hiding this comment.
TIL you can do this in proto!
There was a problem hiding this comment.
It's not going to be a great experience if someone mixes different versions of restate-server and restatectl. Not fatal in this case but you might be wondering why you're getting validation errors on a seemingly legit reconfigure request. At some point we should start including explicit version info in these messages.
Should we keep the field on the server and explicitly reject the request as a validation error if params is used for now?
There was a problem hiding this comment.
I get your concern. I definitely missed that one. My focus was to make sure it doesn't do harm if `params is missing and the system will just reconfigure based on the cluster configuration.
I like your idea. I will put this field back and just fail if it's set
| .preferred_sequencer(&self.log_id) | ||
| }); | ||
|
|
||
| // let provider_kind = self.extension.unwrap(log.con) |
There was a problem hiding this comment.
Do we need this?
| // let provider_kind = self.extension.unwrap(log.con) |
There was a problem hiding this comment.
Definitely not! I am blind 😅
|
Thank you @pcholakov for your review. Good catch on the deprecation error. We now return an error if we using an It's unfortunately still possible to use newer version of the tool with older version of the server. it will not cause a problem but will not behave as expected. I will look into always include the client and server version in requests/responses headers as a follow up, so it's always possible for both the server and the client to decide if the payload should be accepted or not. |
| // [deprecated] We return an error if this | ||
| // field is set. |
There was a problem hiding this comment.
Nice! In the future we could require an explicit version to be passed in, and then we can actually drop this.
| // `params`` is no longer supported. It's better to fail loudly | ||
| // then act unexpectedly on invalid request version | ||
| return Err(Status::invalid_argument( | ||
| "Detected a deprecated argument. Please upgrade to the latest version of the command-line tool to ensure compatibility.", |
There was a problem hiding this comment.
Maybe better to be explicit about which tool we mean?
| "Detected a deprecated argument. Please upgrade to the latest version of the command-line tool to ensure compatibility.", | |
| "Detected a deprecated argument. Please upgrade to the latest version of the restatectl tool to ensure compatibility.", |
| Some(ext) => { | ||
| if !ext.params.is_empty() { | ||
| // `params`` is no longer supported. It's better to fail loudly | ||
| // then act unexpectedly on invalid request version |
There was a problem hiding this comment.
nit
| // then act unexpectedly on invalid request version | |
| // than act unexpectedly on invalid request version |
Summary: - Arguments provided to this command (in case of replicated provider) act as "hints" to the node to consider during replication. It still possible that the node decides to use more nodes or different than the provided nodeset - `sequencer` can also be a PlainNodeId - Most arguments are optional in case previous loglet is a replicated loglet, Otherwise the user needs to provide most arguments
Harden
restatectl reconfigureand avoid sending full LogletParamsSummary:
the node to consider during replication. It still possible that the node decides to
use more nodes or different than the provided nodeset
sequencercan also be a PlainNodeIdto provide most arguments