Adding a way to delete backfill runs #207
Conversation
mpawliszyn
force-pushed
the
mikepaw.delete-runs
branch
from
e288e96 to
1cda513
Compare
| @@ -181,5 +169,21 @@ class BackfillCreator @Inject constructor( | |||
|
|
|||
| companion object { | |||
| private val logger = getLogger<BackfillCreator>() | |||
|
|
|||
| fun PrepareBackfillResponse.validate() { | |||
There was a problem hiding this comment.
why extension instead of a method that takes it in?
| when (desiredState) { | ||
| RUNNING -> slackHelper.runStarted(Id(id), caller.principal) | ||
| PAUSED -> slackHelper.runPaused(Id(id), caller.principal) | ||
| CANCELLED -> slackHelper.runCancelled(Id(id), caller.principal) |
There was a problem hiding this comment.
do you really want to notify? I wouldn't think anyone cares
| PAUSED -> "stopped" | ||
| RUNNING -> "started" | ||
| CANCELLED -> "cancelled" | ||
| else -> desiredState.name |
There was a problem hiding this comment.
not reachable right? throw exception instead?
| /** | ||
| * When the Backfill state changes modify the underlying partitions to these corresponding states. | ||
| */ | ||
| fun BackfillState.getPartitionState() = when (this) { |
| CANCELLED; // A backfill that has been manually cancelled, this is a final non-resumable state. | ||
|
|
||
| companion object { | ||
| val FINAL_STATES = setOf(CANCELLED, COMPLETE) |
There was a problem hiding this comment.
TERMINAL might be a better word
There was a problem hiding this comment.
however, either is fine
| // State can't be changed after being completed. | ||
| checkState(this.state != BackfillState.COMPLETE) | ||
| // Backfills in final states cannot change. | ||
| checkState(!BackfillState.FINAL_STATES.contains(this.state)) |
There was a problem hiding this comment.
could instead do
this.state !in BackfillState.FINAL_STATES
| query.setParameter("newState", state) | ||
| query.setParameter("completed", BackfillState.COMPLETE) | ||
| query.setParameter("newPartitionState", state.getPartitionState()) | ||
| query.setParameterList("finalPartitionStates", BackfillPartitionState.FINAL_STATES) |
There was a problem hiding this comment.
cool this works well. I was worried it would harder to read
| } | ||
|
|
||
| @Test fun `cancel newly created backfill`() { | ||
| val createdBackfill = createBackfillRun(false) |
There was a problem hiding this comment.
use named arg for this boolean
| } | ||
|
|
||
| transacter.transaction { session -> | ||
| val run = queryFactory.newQuery<BackfillRunQuery>().uniqueResult(session) |
There was a problem hiding this comment.
can we use the status api instead of transaction?
| } | ||
| } | ||
|
|
||
| @Nested inner class `And then the backfill is started` { |
There was a problem hiding this comment.
Yeah this is where I overdid it a bit I think. I was trying out some new stuff.
There was a problem hiding this comment.
lol it's a lil awkward to me, can we just call setupOtherThing() as the first line in the tests that need extra setup?
| scope.fakeCaller(user = "molly") { | ||
| var backfillRuns = getBackfillRunsAction.backfillRuns("deep-fryer") | ||
| assertThat(backfillRuns.paused_backfills).hasSize(0) | ||
| assertThat(backfillRuns.running_backfills).hasSize(0) | ||
| softAssert { |
There was a problem hiding this comment.
soft assert gives you all the errors instead of just the first one.
WIP