TSPS-361 add quota consumed logic #160
Conversation
|
hmm im not sure how to suppress that sonarqube issue 🤔 |
| */ | ||
| public UserQuota updateQuotaConsumed(UserQuota userQuota, int newQuotaConsumed) { | ||
| if (newQuotaConsumed < 0) { | ||
| throw new InternalServerErrorException("Quota consumed must be greater than or equal to 0"); |
There was a problem hiding this comment.
I don't remember, do these messages get returned to the user in the api error response?
There was a problem hiding this comment.
it does get returned to the user
{
"jobReport": {
"id": "3fa85f64-5717-4562-0009-2c963f66afa6",
"description": "fail if quota < 1000",
"status": "FAILED",
"statusCode": 500,
"submitted": "2024-11-18T17:29:48.746984Z",
"completed": "2024-11-18T17:32:01.323572Z",
"resultURL": "http://localhost:8080/api/pipelineruns/v1/array_imputation/start/result/3fa85f64-5717-4562-0009-2c963f66afa6"
},
"errorReport": {
"message": "Quota consumed must be greater than or equal to 0",
"statusCode": 500,
"causes": []
},
"pipelineRunReport": {
"pipelineName": "array_imputation",
"pipelineVersion": 0,
"wdlMethodVersion": "ImputationBeagle_development_v0.0.1"
}
}
maybe instead we should log errors and the string here should be somethign more generic like "Internal Error" ?
There was a problem hiding this comment.
hmm yeah i feel like we'll want a way to distinguish error messages that we want the user to see (like the next one) and hide the truly internal ones (like this one). i think we maybe have a ticket for that already: https://broadworkbench.atlassian.net/browse/TSPS-340 but i think that means that the one a few lines down (InternalServerErrorException("Quota consumed cannot be greater than user quota");) should maybe not be an InternalServerErrorException ?
There was a problem hiding this comment.
oh I see, this method shouldn't be called at all if the quota consumed is greater than user quota. so that's accurately an InternalServerErrorException.
I'm on board! no change needed now
| @@ -52,6 +49,9 @@ public StepResult doStep(FlightContext flightContext) { | |||
| inputParameters.get(ImputationJobMapKeys.CONTROL_WORKSPACE_NAME, String.class); | |||
| PipelinesEnum pipelineName = inputParameters.get(JobMapKeys.PIPELINE_NAME, PipelinesEnum.class); | |||
|
|
|||
| // validate and extract parameters from working map | |||
There was a problem hiding this comment.
nit: we're not extracting anything, just doing this to be ready to receive the quotaConsumed
| import bio.terra.stairway.*; | ||
|
|
||
| /** | ||
| * This step calls Rawls to fetch outputs from a data table row for a given quota consumed job. It |
There was a problem hiding this comment.
need to update this description
| if (totalQuotaConsumed > userQuota.getQuota()) { | ||
| return new StepResult( | ||
| StepStatus.STEP_RESULT_FAILURE_FATAL, | ||
| new BadRequestException("User quota exceeded for pipeline " + pipelineName.getValue())); |
There was a problem hiding this comment.
is this the error message that the user will see? should we give more info here, like about how much quota they had, what they were trying to consume, how to request more?
There was a problem hiding this comment.
yea will add some more stuff here. do we have an email yet for them to contact for this?
There was a problem hiding this comment.
updated it with more info and our developer email (for now). the resposne now looks liek
{
"jobReport": {
"id": "3fa85f64-5717-4562-000c-2c963f66afa6",
"description": "testing over quota limit logic",
"status": "FAILED",
"statusCode": 400,
"submitted": "2024-11-18T17:58:23.799295Z",
"completed": "2024-11-18T18:00:35.473919Z",
"resultURL": "http://localhost:8080/api/pipelineruns/v1/array_imputation/start/result/3fa85f64-5717-4562-000c-2c963f66afa6"
},
"errorReport": {
"message": "User quota exceeded for pipeline array_imputation. User quota: 120, Quota consumed so far: 100, Quota consumed for this run: 50. If you would like to request a quota increase, you can email [email protected] ",
"statusCode": 400,
"causes": []
},
"pipelineRunReport": {
"pipelineName": "array_imputation",
"pipelineVersion": 0,
"wdlMethodVersion": "ImputationBeagle_development_v0.0.1"
}
}
There was a problem hiding this comment.
so much better, but it wasn't obvious to me that "user quota" is the actual limit. I think it'd be slightly more clear to say
User quota exceeded for pipeline array_imputation. User quota limit: 120, Quota consumed before this run: 100, Quota consumed for this run: 50. If you would like to request a quota increase, you can email [email protected]
|
|
||
| UserQuota userQuota = quotasService.getQuotaForUserAndPipeline(userId, pipelineName); | ||
|
|
||
| quotasService.updateQuotaConsumed( |
There was a problem hiding this comment.
this is so nice, this here is the justification for separating the fetchQuotaConsumed step from this one.
| import bio.terra.stairway.*; | ||
|
|
||
| /** | ||
| * This step validates that the quota consumed for this run does cause the user to exceed their |
There was a problem hiding this comment.
| * This step validates that the quota consumed for this run does cause the user to exceed their | |
| * This step validates that the quota consumed for this run does not cause the user to exceed their |
| userQuota.getQuota(), | ||
| userQuota.getQuotaConsumed(), | ||
| newQuotaConsumed); | ||
| throw new InternalServerErrorException("Internal Error"); |
There was a problem hiding this comment.
cool this is nice, this is how we want it to work. it's possible that in https://broadworkbench.atlassian.net/browse/TSPS-340 we could do this at a higher level but this is good for now
| new BadRequestException("User quota exceeded for pipeline " + pipelineName.getValue())); | ||
| new BadRequestException( | ||
| String.format( | ||
| "User quota exceeded for pipeline %s. User quota: %d, Quota consumed so far: %d, Quota consumed for" |
There was a problem hiding this comment.
just to be super clear could we make this
User quota exceeded for pipeline %s. User quota limit: %d, Quota consumed before this run: %d, Quota consumed for this run: %d. If you would like to request a quota increase, you can email [email protected]
|
Description
This pr does a few things
Jira Ticket
https://broadworkbench.atlassian.net/browse/TSPS-361