perf(stage): Remove duplicate storeStage call

Daniel Zheng · kirangodishala · commit e566e21094b4 · 2024-11-11T22:04:25.000+05:30
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.
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,7 +793,7 @@ 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)
               }
@@ -833,15 +833,15 @@ 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)
               }
@@ -881,15 +881,15 @@ 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)
               }
@@ -953,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())
@@ -962,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,7 +793,7 @@ 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`	`}`
`@@ -833,15 +833,15 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`833`	`833`	`}`
`834`	`834`
`835`	`835`	`it("attaches the exception to the stage context") {`
`836`		`- verify(repository, times(2)).storeStage(`
	`836`	`+ verify(repository).storeStage(`
`837`	`837`	`check {`
`838`	`838`	`assertThat(it.context["exception"]).isEqualTo(exceptionDetails)`
`839`	`839`	`}`
`840`	`840`	`)`
`841`	`841`	`}`
`842`	`842`
`843`	`843`	`it("attaches flag to the stage context to indicate that before stage planning failed") {`
`844`		`- verify(repository, times(2)).storeStage(`
	`844`	`+ verify(repository).storeStage(`
`845`	`845`	`check {`
`846`	`846`	`assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true)`
`847`	`847`	`}`
`@@ -881,15 +881,15 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`881`	`881`	`}`
`882`	`882`
`883`	`883`	`it("attaches the exception to the stage context") {`
`884`		`- verify(repository, times(2)).storeStage(`
	`884`	`+ verify(repository).storeStage(`
`885`	`885`	`check {`
`886`	`886`	`assertThat(it.context["exception"]).isEqualTo(exceptionDetails)`
`887`	`887`	`}`
`888`	`888`	`)`
`889`	`889`	`}`
`890`	`890`
`891`	`891`	`it("attaches flag to the stage context to indicate that before stage planning failed") {`
`892`		`- verify(repository, times(2)).storeStage(`
	`892`	`+ verify(repository).storeStage(`
`893`	`893`	`check {`
`894`	`894`	`assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true)`
`895`	`895`	`}`
`@@ -953,7 +953,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`953`	`953`	`}`
`954`	`954`
`955`	`955`	`it("updates the stage status") {`
`956`		`- verify(repository, times(2)).storeStage(`
	`956`	`+ verify(repository).storeStage(`
`957`	`957`	`check {`
`958`	`958`	`assertThat(it.status).isEqualTo(RUNNING)`
`959`	`959`	`assertThat(it.startTime).isEqualTo(clock.millis())`
`@@ -962,7 +962,7 @@ object StartStageHandlerTest : SubjectSpek<StartStageHandler>({`
`962`	`962`	`}`
`963`	`963`
`964`	`964`	`it("attaches tasks to the stage") {`
`965`		`- verify(repository, times(2)).storeStage(`
	`965`	`+ verify(repository).storeStage(`
`966`	`966`	`check {`
`967`	`967`	`assertThat(it.tasks.size).isEqualTo(1)`
`968`	`968`	`it.tasks.first().apply {`