perf(repository): Remove duplicate storeStage call (#4795)

kirangodishala · Daniel Zheng · web-flow · commit 9e2d2c608d54 · 2024-11-11T23:55:08.000Z
* test(handler): Verify that start time is set Verify that the start time for a stage is set even when we encounter an exception * perf(stage): Remove duplicate storeStage call The duplicate repository.storeStage call was originally added in e872ce8 to ensure that the start time for a stage is set even if the handler encounters an exception. However, even without the extra repository.storeStage, orca still sets the start time for a stage when it encounters an exception. --------- Co-authored-by: Daniel Zheng <d.zheng@salesforce.com>
diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandler.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandler.kt
@@ -95,7 +95,6 @@ class StartStageHandler(
               try {
                 // Set the startTime in case we throw an exception.
                 stage.startTime = clock.millis()
-                repository.storeStage(stage)
                 stage.plan()
                 stage.status = RUNNING
                 repository.storeStage(stage)
diff --git a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt
@@ -162,7 +162,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       }
 
       it("updates the stage status") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.status).isEqualTo(RUNNING)
             assertThat(it.startTime).isEqualTo(clock.millis())
@@ -171,7 +171,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       }
 
       it("attaches tasks to the stage") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.tasks.size).isEqualTo(1)
             it.tasks.first().apply {
@@ -222,7 +222,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
         }
 
         it("updates the stage status") {
-          verify(repository, times(2)).storeStage(
+          verify(repository).storeStage(
             check {
               assertThat(it.status).isEqualTo(RUNNING)
               assertThat(it.startTime).isEqualTo(clock.millis())
@@ -267,7 +267,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
         }
 
         it("updates the stage status") {
-          verify(repository, times(2)).storeStage(
+          verify(repository).storeStage(
             check {
               assertThat(it.status).isEqualTo(RUNNING)
               assertThat(it.startTime).isEqualTo(clock.millis())
@@ -312,7 +312,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       afterGroup(::resetMocks)
 
       it("attaches tasks to the stage") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.tasks.size).isEqualTo(3)
             it.tasks[0].apply {
@@ -702,7 +702,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       }
 
       it("starts the stage") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.type).isEqualTo("bar")
             assertThat(it.status).isEqualTo(RUNNING)
@@ -712,7 +712,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       }
 
       it("attaches a task to the stage") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.tasks.size).isEqualTo(1)
             it.tasks.first().apply {
@@ -793,12 +793,20 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
           }
 
           it("attaches the exception to the stage context") {
-            verify(repository, times(2)).storeStage(
+            verify(repository).storeStage(
               check {
                 assertThat(it.context["exception"]).isEqualTo(exceptionDetails)
               }
             )
           }
+
+          it("updates the stage with a non-default start time") {
+            verify(repository).storeStage(
+              check {
+                assertThat(it.startTime).isPositive()
+              }
+            )
+          }
         }
 
         and("only the branch should fail") {
@@ -825,20 +833,28 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
           }
 
           it("attaches the exception to the stage context") {
-            verify(repository, times(2)).storeStage(
+            verify(repository).storeStage(
               check {
                 assertThat(it.context["exception"]).isEqualTo(exceptionDetails)
               }
             )
           }
 
           it("attaches flag to the stage context to indicate that before stage planning failed") {
-            verify(repository, times(2)).storeStage(
+            verify(repository).storeStage(
               check {
                 assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true)
               }
             )
           }
+
+          it("updates the stage with a non-default start time") {
+            verify(repository).storeStage(
+              check {
+                assertThat(it.startTime).isPositive()
+              }
+            )
+          }
         }
 
         and("the branch should be allowed to continue") {
@@ -865,20 +881,28 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
           }
 
           it("attaches the exception to the stage context") {
-            verify(repository, times(2)).storeStage(
+            verify(repository).storeStage(
               check {
                 assertThat(it.context["exception"]).isEqualTo(exceptionDetails)
               }
             )
           }
 
           it("attaches flag to the stage context to indicate that before stage planning failed") {
-            verify(repository, times(2)).storeStage(
+            verify(repository).storeStage(
               check {
                 assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true)
               }
             )
           }
+
+          it("updates the stage with a non-default start time") {
+            verify(repository).storeStage(
+              check {
+                assertThat(it.startTime).isPositive()
+              }
+            )
+          }
         }
       }
 
@@ -929,7 +953,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       }
 
       it("updates the stage status") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.status).isEqualTo(RUNNING)
             assertThat(it.startTime).isEqualTo(clock.millis())
@@ -938,7 +962,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({
       }
 
       it("attaches tasks to the stage") {
-        verify(repository, times(2)).storeStage(
+        verify(repository).storeStage(
           check {
             assertThat(it.tasks.size).isEqualTo(1)
             it.tasks.first().apply {

Original file line number	Diff line number	Diff line change
`@@ -162,7 +162,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`162`	`162`	`}`
`163`	`163`
`164`	`164`	`it("updates the stage status") {`
`165`		`- verify(repository, times(2)).storeStage(`
	`165`	`+ verify(repository).storeStage(`
`166`	`166`	`check {`
`167`	`167`	`assertThat(it.status).isEqualTo(RUNNING)`
`168`	`168`	`assertThat(it.startTime).isEqualTo(clock.millis())`
`@@ -171,7 +171,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`171`	`171`	`}`
`172`	`172`
`173`	`173`	`it("attaches tasks to the stage") {`
`174`		`- verify(repository, times(2)).storeStage(`
	`174`	`+ verify(repository).storeStage(`
`175`	`175`	`check {`
`176`	`176`	`assertThat(it.tasks.size).isEqualTo(1)`
`177`	`177`	`it.tasks.first().apply {`
`@@ -222,7 +222,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`222`	`222`	`}`
`223`	`223`
`224`	`224`	`it("updates the stage status") {`
`225`		`- verify(repository, times(2)).storeStage(`
	`225`	`+ verify(repository).storeStage(`
`226`	`226`	`check {`
`227`	`227`	`assertThat(it.status).isEqualTo(RUNNING)`
`228`	`228`	`assertThat(it.startTime).isEqualTo(clock.millis())`
`@@ -267,7 +267,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`267`	`267`	`}`
`268`	`268`
`269`	`269`	`it("updates the stage status") {`
`270`		`- verify(repository, times(2)).storeStage(`
	`270`	`+ verify(repository).storeStage(`
`271`	`271`	`check {`
`272`	`272`	`assertThat(it.status).isEqualTo(RUNNING)`
`273`	`273`	`assertThat(it.startTime).isEqualTo(clock.millis())`
`@@ -312,7 +312,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`312`	`312`	`afterGroup(::resetMocks)`
`313`	`313`
`314`	`314`	`it("attaches tasks to the stage") {`
`315`		`- verify(repository, times(2)).storeStage(`
	`315`	`+ verify(repository).storeStage(`
`316`	`316`	`check {`
`317`	`317`	`assertThat(it.tasks.size).isEqualTo(3)`
`318`	`318`	`it.tasks[0].apply {`
`@@ -702,7 +702,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`702`	`702`	`}`
`703`	`703`
`704`	`704`	`it("starts the stage") {`
`705`		`- verify(repository, times(2)).storeStage(`
	`705`	`+ verify(repository).storeStage(`
`706`	`706`	`check {`
`707`	`707`	`assertThat(it.type).isEqualTo("bar")`
`708`	`708`	`assertThat(it.status).isEqualTo(RUNNING)`
`@@ -712,7 +712,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`712`	`712`	`}`
`713`	`713`
`714`	`714`	`it("attaches a task to the stage") {`
`715`		`- verify(repository, times(2)).storeStage(`
	`715`	`+ verify(repository).storeStage(`
`716`	`716`	`check {`
`717`	`717`	`assertThat(it.tasks.size).isEqualTo(1)`
`718`	`718`	`it.tasks.first().apply {`
`@@ -793,12 +793,20 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`793`	`793`	`}`
`794`	`794`
`795`	`795`	`it("attaches the exception to the stage context") {`
`796`		`- verify(repository, times(2)).storeStage(`
	`796`	`+ verify(repository).storeStage(`
`797`	`797`	`check {`
`798`	`798`	`assertThat(it.context["exception"]).isEqualTo(exceptionDetails)`
`799`	`799`	`}`
`800`	`800`	`)`
`801`	`801`	`}`
	`802`	`+`
	`803`	`+ it("updates the stage with a non-default start time") {`
	`804`	`+ verify(repository).storeStage(`
	`805`	`+ check {`
	`806`	`+ assertThat(it.startTime).isPositive()`
	`807`	`+ }`
	`808`	`+ )`
	`809`	`+ }`
`802`	`810`	`}`
`803`	`811`
`804`	`812`	`and("only the branch should fail") {`
`@@ -825,20 +833,28 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`825`	`833`	`}`
`826`	`834`
`827`	`835`	`it("attaches the exception to the stage context") {`
`828`		`- verify(repository, times(2)).storeStage(`
	`836`	`+ verify(repository).storeStage(`
`829`	`837`	`check {`
`830`	`838`	`assertThat(it.context["exception"]).isEqualTo(exceptionDetails)`
`831`	`839`	`}`
`832`	`840`	`)`
`833`	`841`	`}`
`834`	`842`
`835`	`843`	`it("attaches flag to the stage context to indicate that before stage planning failed") {`
`836`		`- verify(repository, times(2)).storeStage(`
	`844`	`+ verify(repository).storeStage(`
`837`	`845`	`check {`
`838`	`846`	`assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true)`
`839`	`847`	`}`
`840`	`848`	`)`
`841`	`849`	`}`
	`850`	`+`
	`851`	`+ it("updates the stage with a non-default start time") {`
	`852`	`+ verify(repository).storeStage(`
	`853`	`+ check {`
	`854`	`+ assertThat(it.startTime).isPositive()`
	`855`	`+ }`
	`856`	`+ )`
	`857`	`+ }`
`842`	`858`	`}`
`843`	`859`
`844`	`860`	`and("the branch should be allowed to continue") {`
`@@ -865,20 +881,28 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`865`	`881`	`}`
`866`	`882`
`867`	`883`	`it("attaches the exception to the stage context") {`
`868`		`- verify(repository, times(2)).storeStage(`
	`884`	`+ verify(repository).storeStage(`
`869`	`885`	`check {`
`870`	`886`	`assertThat(it.context["exception"]).isEqualTo(exceptionDetails)`
`871`	`887`	`}`
`872`	`888`	`)`
`873`	`889`	`}`
`874`	`890`
`875`	`891`	`it("attaches flag to the stage context to indicate that before stage planning failed") {`
`876`		`- verify(repository, times(2)).storeStage(`
	`892`	`+ verify(repository).storeStage(`
`877`	`893`	`check {`
`878`	`894`	`assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true)`
`879`	`895`	`}`
`880`	`896`	`)`
`881`	`897`	`}`
	`898`	`+`
	`899`	`+ it("updates the stage with a non-default start time") {`
	`900`	`+ verify(repository).storeStage(`
	`901`	`+ check {`
	`902`	`+ assertThat(it.startTime).isPositive()`
	`903`	`+ }`
	`904`	`+ )`
	`905`	`+ }`
`882`	`906`	`}`
`883`	`907`	`}`
`884`	`908`
`@@ -929,7 +953,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`929`	`953`	`}`
`930`	`954`
`931`	`955`	`it("updates the stage status") {`
`932`		`- verify(repository, times(2)).storeStage(`
	`956`	`+ verify(repository).storeStage(`
`933`	`957`	`check {`
`934`	`958`	`assertThat(it.status).isEqualTo(RUNNING)`
`935`	`959`	`assertThat(it.startTime).isEqualTo(clock.millis())`
`@@ -938,7 +962,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`938`	`962`	`}`
`939`	`963`
`940`	`964`	`it("attaches tasks to the stage") {`
`941`		`- verify(repository, times(2)).storeStage(`
	`965`	`+ verify(repository).storeStage(`
`942`	`966`	`check {`
`943`	`967`	`assertThat(it.tasks.size).isEqualTo(1)`
`944`	`968`	`it.tasks.first().apply {`