Conversation
chenzl25
left a comment
There was a problem hiding this comment.
I found two blocking issues on the current head:
| origin: Vec<WorkerNode>, | ||
| worker_node_mask: &HashSet<WorkerId>, | ||
| ) -> Vec<WorkerNode> { | ||
| if origin.iter().all(|w| worker_node_mask.contains(&w.id)) { |
There was a problem hiding this comment.
Blocking: this fallback now also bypasses the version check. If every serving worker has a different/empty version, or the only matching-version worker is temporarily masked, all(...) is true and we return the original list, making incompatible workers schedulable again. Please keep version mismatch fail-closed and apply the “all masked” fallback only to the temporary mask within the matching-version subset. Please also cover both cases in tests.
There was a problem hiding this comment.
BTW for a typical serving node (frontend + serving role compute node), at least 1 matched serving node is guaranteed.
| fn effective_worker_node_mask(&self, serving_worker_nodes: &[WorkerNode]) -> HashSet<WorkerId> { | ||
| let mut mask = self.worker_node_mask.read().unwrap().clone(); | ||
| mask.extend(serving_worker_nodes.iter().filter_map(|worker| { | ||
| (worker.resource.as_ref().map(|r| r.rw_version.as_str()) != Some(RW_VERSION)) |
There was a problem hiding this comment.
Blocking: resource.rw_version is not guaranteed to be current in the frontend snapshot. WorkerNode.resource is not persisted; after meta restarts, existing workers are rebuilt with default resource data, while heartbeat updates only meta memory and the frontend handles only Add/Delete node notifications. A frontend snapshot taken before the first heartbeat can therefore retain an empty version and mask healthy workers indefinitely. Please make version updates durable/observable before using this field as a scheduling filter.
There was a problem hiding this comment.
Will make WorkerNode.resource durable.
|
Will merge this PR after #26568 |
# Conflicts: # src/batch/src/worker_manager/worker_node_manager.rs # src/meta/src/controller/cluster.rs
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR improves availability during rolling upgrades when serving nodes run differing RW versions.
Performance overhead justification:
Details:
RW_VERSIONbefore remapping serving fragment mappings.Checklist
Documentation
Release note