SEZ9 commented on issue #12087: URL: https://github.com/apache/seatunnel/issues/12087#issuecomment-5564553625
Thanks @JeremyXin for clarifying the intent. I agree the PR stays advisory-only: it does not invoke actuators, mutate worker capacity, drain workers, or rescale running jobs. However, the boundary recorded in this thread was narrower than the complete Phase 1 milestone in one PR. The agreed first slice was server-local only: the immutable snapshot/recommendation models, pure policy evaluator, monotonic-clock stabilization tracker, safe signal collection, fencing/idempotency, and the deterministic contract tests, all under the server-local autoscale package in seatunnel-engine-server. Engine-common configuration, YAML/default-config changes, REST endpoints, Prometheus exports, and user documentation were deferred to a later integration PR. That also matches the acceptance order @CryoThrust suggested: get the value objects and policy evaluator reviewed and tested independently of the adapters, then project the same evaluated snapshot into REST/metrics. The reason to keep that split is practical: reviewing decision logic, fencing, and stabilization semantics is much easier when the diff is not also introducing public configuration keys and REST/metric surfaces we will need to keep compatible afterwards. Landing the tested model first also lets the follow-up PR be reviewed purely as a projection. Concrete asks: 1. Please split the PR so the first one contains only the server-local package (models, evaluator, stabilization tracker, signal collection, fencing/idempotency) plus the deterministic tests listed earlier: epoch/generation fencing, duplicate advisory idempotency, Fixed vs Dynamic Slot semantics, stale/future worker samples, missing metrics blocking scale-in, signal conflicts, and active-master handover reset. 2. Move the engine-common configuration, default config/YAML parsing, REST status/metrics/history services and servlets, Prometheus exports, and documentation into a follow-up integration PR that depends on the first one. 3. If you read the STIP as requiring the full milestone to land in a single PR, please quote the relevant section here so we can reconcile it with the boundary agreed in this thread before review continues. Once the split is in place I will review the first PR against the Phase 1 requirements, and since you have invited @CryoThrust to review, that would be welcome too. <!-- streview-comment:863 --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
