Conversation
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds publisher-based step lifecycle support: two new publisher interfaces, a new AbstractStepExecutor centralizing RUNNING/COMPLETED/FAILED/SKIPPED transitions, refactors StepExecutionService to use those publishers, and moves ReportInfos to the commons package. Changes
Sequence DiagramsequenceDiagram
participant Service as StepExecutionService
participant Executor as AbstractStepExecutor
participant StatusPub as StepStatusPublisher
participant ReportPub as ReportPublisher
Service->>Executor: executeStep(processId, stepId, name, order, startedAt, reportUuid, reportInfos, resultInfos, stepExecution)
activate Executor
Executor->>StatusPub: updateStepStatus(..., RUNNING)
activate StatusPub
StatusPub-->>Executor: ack
deactivate StatusPub
Executor->>Executor: run stepExecution
alt Success
Executor->>ReportPub: sendReport(reportInfos)
activate ReportPub
ReportPub-->>Executor: ack
deactivate ReportPub
Executor->>StatusPub: updateStepStatus(..., COMPLETED)
else Failure
Executor->>StatusPub: updateStepStatus(..., FAILED + error)
end
activate StatusPub
StatusPub-->>Executor: ack
deactivate StatusPub
deactivate Executor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java (1)
68-69:context.getReportInfos()called twice — cache in a local variable.On Line 68,
Objects.requireNonNull(context.getReportInfos())validates and discards the reference; Line 69 calls it again. Cache the result so the null guard and the value use the same object reference.♻️ Proposed fix
public void executeStep(ProcessStepExecutionContext<C> context, ProcessStep<C> step) { setExecutionId(context.getProcessExecutionId()); + ReportInfos reportInfos = Objects.requireNonNull(context.getReportInfos()); executeStep( context.getStepExecutionId(), step.getType().getName(), context.getStepOrder(), context.getStartedAt(), - Objects.requireNonNull(context.getReportInfos()).reportUuid(), - context.getReportInfos(), + reportInfos.reportUuid(), + reportInfos, context.getResultInfos(), () -> step.execute(context) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java` around lines 68 - 69, The code calls context.getReportInfos() twice—once inside Objects.requireNonNull(...) and again on the next line—so cache the value in a local variable (e.g., ReportInfos reportInfos = Objects.requireNonNull(context.getReportInfos())) and then use reportInfos.reportUuid() and reportInfos for the subsequent arguments; update the block in StepExecutionService where Objects.requireNonNull(context.getReportInfos()) and context.getReportInfos() are used to reference the single cached variable.monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java (1)
65-68:sendReportinside thetryblock couples report-delivery failure to step failure.If
stepExecution.run()succeeds butgetReportPublisher().sendReport(reportInfos)throws on Line 67, the catch on Line 69 fires and the step is markedFAILED— even though the computation completed successfully. Consider whether report-delivery failure should be treated as a step failure or handled separately (e.g., its own retry/dead-letter flow).♻️ Proposed separation of concerns
try { stepExecution.run(); - getReportPublisher().sendReport(reportInfos); updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.COMPLETED); } catch (Exception e) { try { updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED); } catch (Exception statusEx) { e.addSuppressed(statusEx); } throw e; } +// Report delivery is a separate concern; handle its failure independently. +getReportPublisher().sendReport(reportInfos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java` around lines 65 - 68, The call to getReportPublisher().sendReport(reportInfos) is inside the try that surrounds stepExecution.run(), so a report-delivery failure will incorrectly mark the step as FAILED; move or isolate sendReport so it cannot change step outcome: keep stepExecution.run() and updateStepStatus(..., StepStatus.COMPLETED) inside the primary try for the computation, then perform getReportPublisher().sendReport(reportInfos) in a separate try/catch (or a retry/dead-letter flow) that logs or records report-delivery errors without calling updateStepStatus with a failure status; reference stepExecution.run(), getReportPublisher().sendReport(reportInfos), updateStepStatus(...) and StepStatus.COMPLETED when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java`:
- Around line 65-73: When handling failures in the try/catch around
stepExecution.run(), ensure the original exception from stepExecution.run() is
not lost if updateStepStatus(...) throws: inside the catch(Exception e) block
catch and suppress any exception thrown by updateStepStatus(stepExecutionId,
stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED)
(optionally logging it via the same logger/reporting mechanism) and then rethrow
the original exception `e`; i.e., wrap the call to updateStepStatus in its own
try/catch so updateStepStatus failures cannot overwrite the original exception
thrown by stepExecution.run().
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java`:
- Around line 27-31: StepExecutionService stores executionId as instance state
causing a race between setExecutionId(...) and the interface default methods
(skipStep/executeStep) that call getExecutionId(); fix by removing shared
mutable state: either (A) replace the executionId field with a ThreadLocal<UUID>
(e.g., executionIdHolder) and ensure you call executionIdHolder.remove() in a
finally block after each step, updating setExecutionId/getExecutionId to use the
ThreadLocal, or (B, preferred) change the StepExecutionInterface API to remove
getExecutionId()/setExecutionId() and add UUID executionId as the first
parameter to skipStep and executeStep and propagate that parameter through all
implementing classes (including StepExecutionService) so no singleton holds
per-thread state.
---
Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java`:
- Around line 65-68: The call to getReportPublisher().sendReport(reportInfos) is
inside the try that surrounds stepExecution.run(), so a report-delivery failure
will incorrectly mark the step as FAILED; move or isolate sendReport so it
cannot change step outcome: keep stepExecution.run() and updateStepStatus(...,
StepStatus.COMPLETED) inside the primary try for the computation, then perform
getReportPublisher().sendReport(reportInfos) in a separate try/catch (or a
retry/dead-letter flow) that logs or records report-delivery errors without
calling updateStepStatus with a failure status; reference stepExecution.run(),
getReportPublisher().sendReport(reportInfos), updateStepStatus(...) and
StepStatus.COMPLETED when making this change.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java`:
- Around line 68-69: The code calls context.getReportInfos() twice—once inside
Objects.requireNonNull(...) and again on the next line—so cache the value in a
local variable (e.g., ReportInfos reportInfos =
Objects.requireNonNull(context.getReportInfos())) and then use
reportInfos.reportUuid() and reportInfos for the subsequent arguments; update
the block in StepExecutionService where
Objects.requireNonNull(context.getReportInfos()) and context.getReportInfos()
are used to reference the single cached variable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/ReportPublisher.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepStatusPublisher.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java
Outdated
Show resolved
Hide resolved
...-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
…s' into extract-step-execution-to-commons # Conflicts: # monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/ReportPublisher.java # monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
…s' into extract-step-execution-to-commons Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> # Conflicts: # monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/ReportPublisher.java # monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
…s' into extract-step-execution-to-commons
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecution.java`:
- Around line 46-55: The code captures ResultInfos too early in executeStep (it
passes the resultInfos object before calling stepExecution.run()), causing
stale/null resultUUID/resultType in status updates; modify executeStep to defer
reading/result retrieval of ResultInfos until after stepExecution.run()
completes (or when building the COMPLETED/FAILED status payload), e.g., replace
passing the pre-captured ResultInfos into the status publisher with a fresh read
or Supplier/closure that calls the ResultInfos getters post-run; apply the same
change pattern in StepExecutionService methods that build status updates between
lines ~72-90 so resultUUID/resultType are read after stepExecution.run().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
monitor-commons/pom.xmlmonitor-commons/src/main/java/org/gridsuite/monitor/commons/ReportInfos.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/ReportPublisher.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecution.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContext.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ReportService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/LoadNetworkStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ReportServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/StepExecutionServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
- monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/StepExecutionServiceTest.java
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecution.java
Outdated
Show resolved
Hide resolved
TheMaskedTurtle
left a comment
There was a problem hiding this comment.
Small comments on how the StepExecution is designed but the overall design is good to me !
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/AbstractStepExecutor.java
Show resolved
Hide resolved
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecution.java
Outdated
Show resolved
Hide resolved
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecution.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@thangqp is working on a big reorganization of our files to be able to share code through interfaces (#46). It kind of get out of control though and we're both on vacation next week. So I would suggest that if you want to move forward you can merge this! Sorry I forgot to synchronize with you on this...
Anyway we'll need to publish the lib too
...-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
Outdated
Show resolved
Hide resolved
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecution.java
Outdated
Show resolved
Hide resolved
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
…n-to-commons # Conflicts: # monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java (1)
33-40: Narrow overridden accessor visibility toprotected.These overrides don’t need to be part of the public service API; keeping them
protectedbetter matches the abstraction boundary.♻️ Proposed refactor
- public StepStatusPublisher getStepStatusPublisher() { + protected StepStatusPublisher getStepStatusPublisher() { return stepStatusPublisher; } @@ - public ReportPublisher getReportPublisher() { + protected ReportPublisher getReportPublisher() { return reportPublisher; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java` around lines 33 - 40, The overrides getStepStatusPublisher and getReportPublisher in StepExecutionService are public but should be narrowed to protected; change their method visibility from public to protected for both getStepStatusPublisher() (returning stepStatusPublisher) and getReportPublisher() (returning reportPublisher) so they are not part of the public service API, and adjust any internal callers or tests in the codebase that rely on these being public to use package/protected access or refactor them to use the service's public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/AbstractStepExecutor.java`:
- Around line 46-55: The code in executeStep currently captures/snapshots the
ResultInfos before invoking stepExecution.run(), causing fields set during
execution to be missing from the COMPLETED/FAILED status updates; change the
logic in executeStep (and the other analogous block referenced at lines 72-90)
to defer reading or serializing ResultInfos until after stepExecution.run()
completes (or after catching exceptions) so the status update uses the latest
ResultInfos values, e.g., call getters/serialize ResultInfos only when updating
to COMPLETED or FAILED rather than before calling stepExecution.run().
---
Nitpick comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java`:
- Around line 33-40: The overrides getStepStatusPublisher and getReportPublisher
in StepExecutionService are public but should be narrowed to protected; change
their method visibility from public to protected for both
getStepStatusPublisher() (returning stepStatusPublisher) and
getReportPublisher() (returning reportPublisher) so they are not part of the
public service API, and adjust any internal callers or tests in the codebase
that rely on these being public to use package/protected access or refactor them
to use the service's public API.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
monitor-commons/pom.xmlmonitor-commons/src/main/java/org/gridsuite/monitor/commons/ReportInfos.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/AbstractStepExecutor.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/ReportPublisher.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContext.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ReportService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/LoadNetworkStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ReportServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/StepExecutionServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/StepExecutionServiceTest.java
- monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/AbstractStepExecutor.java
Show resolved
Hide resolved
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/AbstractStepExecutor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
# Conflicts: # monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java # monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.java
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/AbstractStepExecutor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
…ssConfig interface (#43) Moved modificationUuids from ProcessConfig to ModifyingProcessConfig interface in order to make the ProcessConfig interface more generic. It is required for further developments related to SnapshotRefinerServer's connection to monitor-server Signed-off-by: Kamil MARUT <kamil.marut@rte-france.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com> Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
…s' into extract-step-execution-to-commons
|
|
closed and replaced by #60 du to gpg verification problems |



PR Summary
We need to use the step execution service logic in other services, so common behaviour is extracted into the lib monitor-commons