Arvados

ready for review in 0153e8ec303e63ff75a1a2ce52a05ff910706029 on branch 17841-add-duration. Tests are running at developer-run-tests: #2555

addContainerLine() could also return a containerInfo struct instead of two floats

In costAnalyzer(), crCsv seems like a weird name since it's not really a csv, maybe it should be called crInfo?

Should generateCrCsv() have a "totalDuration += tmpTotalDuration" alongside "totalCost += tmpTotalCost"?

It seems like cost and duration are always retrieved the same way and added to other containerInfo field values at the same time, which might be a hint that all costs and durations should be passed around in this form, e.g., totalCost and totalDuration could be total.cost and total.duration where total is a containerInfo... and perhaps containerInfo is really "consumption" or something like that, since it doesn't necessarily refer to just a single container. Using a (*containerInfo)Add(containerInfo) method instead of a += statement for each field would help avoid future bugs like the missing addition to totalDuration, if indeed that's a bug.

Tom Clegg wrote:

addContainerLine() could also return a containerInfo struct instead of two floats

Done!

In costAnalyzer(), crCsv seems like a weird name since it's not really a csv, maybe it should be called crInfo?

Done!

Should generateCrCsv() have a "totalDuration += tmpTotalDuration" alongside "totalCost += tmpTotalCost"?

Yeah, fixed, good find.

It seems like cost and duration are always retrieved the same way and added to other containerInfo field values at the same time, which might be a hint that all costs and durations should be passed around in this form, e.g., totalCost and totalDuration could be total.cost and total.duration where total is a containerInfo...

Yes! Changed.

and perhaps containerInfo is really "consumption" or something like that, since it doesn't necessarily refer to just a single container. Using a (*containerInfo)Add(containerInfo) method instead of a += statement for each field would help avoid future bugs like the missing addition to totalDuration, if indeed that's a bug.

Yes! Good idea, changed.

I also fixed an offset problem with the TOTAL line in the per-CR csv files, the column positions for duration and cost were off by one.

Ready for another look at 98bdab8fca735201efe2a785b6c20003e1d9058f on branch 17841-add-duration. Tests are running at developer-run-tests: #2559

98bdab8fc LGTM, thanks!

Tom Clegg wrote:

98bdab8fc LGTM, thanks!

Thanks, I updated the doc page (I had forgotten to do that) and merged.