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.

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

[NU-1979] Scenario statuses refactor: scenario status and deployment statuses are decoupled #7566

Merged
merged 25 commits into from
Feb 25, 2025

Conversation

arkadius
Copy link
Member

@arkadius arkadius commented Feb 14, 2025

Describe your changes

This refactor aimed to distinguish between scenario status and the status of scenario deployments.
Scenario status is the combination of local actions (deploy/cancel) and deployments visible on engine side.
Before the change both things were coupled together. DeploymentManager has returned the scenario status. After the change, DM returns only the status of deployments and the process of resolution this statuses to scenario status is done in the Designers core.

API changes:

  • Changes in DeploymentManager interface
    • DeploymentManager has only getScenarioDeploymentsStatuses method (previous getProcessStates returning List[StatusDetails]).
    • Method DeploymentManager.resolve should be removed - this work is done by Designer itself
    • DeploymentManagerInconsistentStateHandlerMixIn mixin should be also removed
    • stateQueryForAllScenariosSupport was renamed to deploymentsStatusesQueryForAllScenariosSupport
    • Other changes:
      • StatusDetails was renamed to DeploymentStatusDetails
      • Fields: externalDeploymentId, externalDeploymentId, attributes, attributes were removed from StatusDetails

Other things that were done:

  • Old ProcessState was renamed to ScenarioStatusDto
  • The process of determining of visual related things was encapsulated into ScenarioStatusPresenter and is done close to Resources instead of in domain services
  • ScenarioStateProvider was split into ScenarioStatusProvider and EngineSideDeploymentStatusesProvider
  • Bofore the change, the process of reconciliation of deployment finished statuses was done ad-hoc during fetching the scenario status, on the DM side. Now it is done in the separated class ScenarioDeploymentReconciler which is invoked periodically

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

Copy link
Contributor

created: #7567
⚠️ Be careful! Snapshot changes are not necessarily the cause of the error. Check the logs.

@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from b48260d to dea343a Compare February 14, 2025 19:54
@github-actions github-actions bot removed ui client client main fe docs labels Feb 14, 2025
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 6d40677 to a6fc258 Compare February 17, 2025 21:59
@github-actions github-actions bot added client client main fe ui labels Feb 17, 2025
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 28b5f28 to ce37f72 Compare February 18, 2025 13:34
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 129b672 to 4d49ac4 Compare February 18, 2025 16:56
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 3081df2 to 5a90c77 Compare February 18, 2025 19:09
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 325c9fb to acf8e43 Compare February 18, 2025 20:50
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch 3 times, most recently from a28e6bf to 28d2486 Compare February 19, 2025 09:24
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 28d2486 to 2d7233f Compare February 19, 2025 09:59
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 5cf00fd to b0ebb97 Compare February 19, 2025 14:17
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from b0ebb97 to 80efd7b Compare February 19, 2025 14:46
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from cd1bcbd to 2f2c925 Compare February 19, 2025 23:25
@github-actions github-actions bot added the docs label Feb 19, 2025
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 2f2c925 to 10dc416 Compare February 20, 2025 07:42
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 10cd096 to 51a10dd Compare February 21, 2025 10:54
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from 51a10dd to 95a3da2 Compare February 21, 2025 10:59
@arkadius arkadius marked this pull request as ready for review February 21, 2025 15:59
@arkadius arkadius force-pushed the scenario-status-resolving-refactor branch from acb4d75 to c7a228d Compare February 21, 2025 16:49
@arkadius arkadius changed the title Scenario status resolving refactor [NU-1979] Scenario statuses refactor: scenario status and deployment statuses are decoupled Feb 21, 2025
@@ -53,7 +53,7 @@ object flinkRestModel {

// NOTE: Flink <1.10 compatibility - JobStatus changed package, so we use String here
@JsonCodec(decodeOnly = true) case class JobOverview(
jid: String,
jid: JobID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to introduce our own FlinkJobId, wrapping either a String or a UUID, instead of relying on a class from the Flink API? If I skimmed through the PR correctly, do we only need the string representation of the job ID?

Copy link
Member Author

Choose a reason for hiding this comment

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

This Flink's JobId is quite stable value class. TBH IMO we should remove the whole flink client related code and use Flink's official client for rest API (RestClient/RestClusterClient).

@arkadius arkadius merged commit 0ba764d into staging Feb 25, 2025
19 of 20 checks passed
@arkadius arkadius deleted the scenario-status-resolving-refactor branch February 25, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client main fe docs ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants