-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GAWB-4026: Initial simple cost calculation #2
base: master
Are you sure you want to change the base?
Conversation
core/src/main/scala/org/broadinstitute/workbench/ccm/JsonCodec.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/broadinstitute/workbench/ccm/pricing/GcpPricing.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/broadinstitute/workbench/ccm/pricing/GcpPricing.scala
Outdated
Show resolved
Hide resolved
0.010931) | ||
assertEquals(r, expectedResponse) | ||
} | ||
res.fold[Unit](e => throw e, identity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to do res == Right(xx)
core/src/main/scala/org/broadinstitute/workbench/ccm/model.scala
Outdated
Show resolved
Hide resolved
|
||
private def getPriceOfCall(call: Call, priceList: PriceList, startTime: Instant, endTime: Instant): Either[String, Double] = { | ||
for { | ||
_ <- if (call.status.asString == "Success") Right(()) else Left(s"Call {name} status was ${call.status.asString}.") // not evaluating workflows that are in flight or Failed or Aborted or whatever |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we not evaluating aborted or failed? They cost money too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I meant to change that to evaluating all terminal statuses, not just successes
|
||
|
||
final case class RuntimeAttributes(cpuNumber: CpuNumber, | ||
disks: Disks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can there be more than one disk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure, I will ask Cromwell people
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer is yes, I'll turn it into a list
} | ||
|
||
private def getCallDuration(call: Call, cromwellStartTime: Instant, cromwellEndTime: Instant): Long = { | ||
// ToDo: add option to ignore preempted calls and just return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't preempted calls cost money too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - this comment was some vestige of a thing we aren't doing anymore (I think earlier we were just going to make the calculation ignore preemptibility for an initial iteration). I'll remove the comment
What's missing: