TSPS-360 Add steps to calculate quota consumed #158
Conversation
| @ConfigurationProperties(prefix = "pipelines.wdl") | ||
| @Getter | ||
| @Setter | ||
| public class WdlPipelineConfiguration { |
There was a problem hiding this comment.
noticing you named this pretty generically. what else do you see adding to this configuration besides quotaConsumedPollingIntervalSeconds?
There was a problem hiding this comment.
not sure but i didnt want it associated with imputation - happy to put it in an already existing configuration that wasnt imputations. I imagine there will be a couple of things we'll want to move out of imputation config to this one once we get our next wdl pipeline.
There was a problem hiding this comment.
yeah sounds good. no need to combine with an existing configuration.
| * specifically fetches the quota consumed value from the data table row using the quota_consumed | ||
| * key | ||
| * | ||
| * <p>This step expects nothign from the working map |
There was a problem hiding this comment.
| * <p>This step expects nothign from the working map | |
| * <p>This step expects nothing from the working map |
| new InternalServerErrorException("Quota consumed is unexpectedly null")); | ||
| } | ||
|
|
||
| logger.info("Quota consumed: {}", quotaConsumed); |
There was a problem hiding this comment.
is the idea that in future we'll store this in the working map to be used by the logic to do the quota check in a subsequent step?
There was a problem hiding this comment.
i think we may do the quota check here or in a subsequent step - thats due to be done in https://broadworkbench.atlassian.net/browse/TSPS-361. I'd prob do it here and rename the step
| QUOTA_CONSUMED_OUTPUT_DEFINITION_LIST, | ||
| pipelineName); | ||
|
|
||
| // if there is a validation response that means the validation failed so return it |
There was a problem hiding this comment.
this is elegant! nice!
| .entityType( | ||
| pipelineName.getValue()) // this must match the configuration the method is set to | ||
| // launch with. Will be addressed in | ||
| // https://broadworkbench.atlassian.net/browse/TSPS-301 |
There was a problem hiding this comment.
TSPS-301 is closed duplicate of TSPS-303, which is complete. can we remove this comment or is there still work to be done that wasn't done with TSPS-303? https://broadworkbench.atlassian.net/browse/TSPS-303
(I'm not really sure what the issue is here that needs to be addressed)
There was a problem hiding this comment.
oh yea this is solved - will remove. This is part of the "magic" validation/updating of the method configuration
| // make sure the step was a success | ||
| assertEquals(StepStatus.STEP_RESULT_SUCCESS, result.getStepStatus()); | ||
| assertEquals( | ||
| testJobId, |
There was a problem hiding this comment.
this shouldn't be the testJobId, should be a different uuid (related to comment above)
| submissionRequestCaptor.capture(), | ||
| eq(TestUtils.CONTROL_WORKSPACE_BILLING_PROJECT), | ||
| eq(TestUtils.CONTROL_WORKSPACE_NAME))) | ||
| .thenReturn(new SubmissionReport().submissionId(testJobId.toString())); |
There was a problem hiding this comment.
it probably doesn't really matter for the test, but to avoid confusion down the line we should make a new randomUUID (like you do in PollQuotaConsumedSubmissionStatusStepTest.java) to use for the submissionId for the quota submission. it's not going to be the same as the flight jobId.
There was a problem hiding this comment.
great point, updating both tests
| any(SubmissionRequest.class), | ||
| eq(TestUtils.CONTROL_WORKSPACE_BILLING_PROJECT), | ||
| eq(TestUtils.CONTROL_WORKSPACE_NAME))) | ||
| .thenReturn(new SubmissionReport().submissionId(testJobId.toString())); |
There was a problem hiding this comment.
same comment here, should use a new uuid
| RawlsService rawlsService, | ||
| SamService samService, |
There was a problem hiding this comment.
just curious why the reorder?
There was a problem hiding this comment.
all the other steps have rawls then sam and it annoyed me this one was different 😄
|
| @ConfigurationProperties(prefix = "pipelines.wdl") | ||
| @Getter | ||
| @Setter | ||
| public class WdlPipelineConfiguration { |
There was a problem hiding this comment.
yeah sounds good. no need to combine with an existing configuration.
Description
This PR adds the logic to calculate how much quota the currently pipeline should consume. It does it by running a "QuotaConsumed" wdl found in the workspace specified to the flight, polling it, and then extracting the value from the data tables.
Successful runs of this flight can be found here. Note now each imputation flight will launch 2 workflows.
Jira Ticket
https://broadworkbench.atlassian.net/browse/TSPS-360