Feature #17717

[costanalyzer] should be able to do its thing for all computation between 2 timestamps

Added by Ward Vandewege 5 months ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
05/26/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Subtasks

Task #17729: review 17717-costanalyzer-date-modeResolvedWard Vandewege

Associated revisions

Revision 2c843ad3
Added by Ward Vandewege 5 months ago

Merge branch '17717-costanalyzer-date-mode'

closes #17717

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

History

#1 Updated by Ward Vandewege 5 months ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 5 months ago

Ready for review in 05607eae460bab7efc61dd33d1e88b31139a97c7 on branch 17717-costanalyzer-date-mode

#3 Updated by Ward Vandewege 5 months ago

  • Target version changed from 2021-05-26 sprint to 2021-06-09 sprint

#4 Updated by Tom Clegg 5 months ago

Usage message: "cost for all supplied uuid." → "...uuids." (or UUIDs?)

Usage message: the explanation of begin/end seems a bit more verbose than necessary, and doesn't mention the somewhat subtle point that only top-level containers' finish times are considered - would this be better? "...it will calculate the cost for all top-level container requests whose container finished during the specified interval."

Nit: "2006-01-02T15:04:05" is repeated several times, should probably be a const

Nit: as a habit, putting "defer close(uuidChannel)" at the top of the go func() (instead of close() at the bottom) makes it less likely that someone will create a deadlock bug in future by inserting an early "return".

Creeping scope of review: IDK if it's worth refactoring right now, but the long list of return values from parseFlags is getting a bit awkward. This could be rearranged so the parameters are fields in the "command" struct, and (*command)parseFlags() modifies them in place. Another existing debt thing is that we seem to create a *config.Loader and pass it around but never actually use it ... and this is a client-side command so we probably never will use it.

Some sort of progress indicator would be good, if we can get one for cheap. Maybe when getting a date range, sort by finished_at, and mention that timestamp in one of the Considering/Processing/Collecting log messages? Come to think of it, merely sorting by uuid would provide a pretty useful progress indicator.

#5 Updated by Ward Vandewege 5 months ago

Tom Clegg wrote:

Usage message: "cost for all supplied uuid." → "...uuids." (or UUIDs?)

Fixed, and I capitalized UUID everywhere in the help text.

Usage message: the explanation of begin/end seems a bit more verbose than necessary, and doesn't mention the somewhat subtle point that only top-level containers' finish times are considered - would this be better? "...it will calculate the cost for all top-level container requests whose container finished during the specified interval."

Thanks, yes, replaced.

Nit: "2006-01-02T15:04:05" is repeated several times, should probably be a const

Fixed.

Nit: as a habit, putting "defer close(uuidChannel)" at the top of the go func() (instead of close() at the bottom) makes it less likely that someone will create a deadlock bug in future by inserting an early "return".

Ah, yes, fixed.

Creeping scope of review: IDK if it's worth refactoring right now, but the long list of return values from parseFlags is getting a bit awkward. This could be rearranged so the parameters are fields in the "command" struct, and (*command)parseFlags() modifies them in place.

Done!

Another existing debt thing is that we seem to create a *config.Loader and pass it around but never actually use it ... and this is a client-side command so we probably never will use it.

Oh, right. Removed.

Some sort of progress indicator would be good, if we can get one for cheap. Maybe when getting a date range, sort by finished_at, and mention that timestamp in one of the Considering/Processing/Collecting log messages? Come to think of it, merely sorting by uuid would provide a pretty useful progress indicator.

Good idea. Sorting by container.finished_at would be ideal, but apparently ordering by a field on the container table is when requesting a list of container_requests. Too bad; it works for filtering. So I did the next best thing, sorting by 'started_at', and I added the timestamp to the 'Collecting child container requests' output line.

Ready for another look at 41e0df276eaaa548e692e44cd8f3c27c4692375e on branch 17717-costanalyzer-date-mode. Tests running at https://ci.arvados.org/view/Developer/job/developer-run-tests/2512/

#6 Updated by Tom Clegg 5 months ago

This should probably be logger.Debugf():

+       fmt.Printf("UUIDS: %s\n", c.uuids)

As an alternative to the SetUpTest func, you could set var Command = command{} and using a value receiver instead of pointer receiver for RunCommand(). Then RunCommand() wouldn't modify the global variable.

Rest LGTM.

#7 Updated by Ward Vandewege 5 months ago

Tom Clegg wrote:

This should probably be logger.Debugf():

[...]

I've just removed it, it was leftover from debugging.

As an alternative to the SetUpTest func, you could set var Command = command{} and using a value receiver instead of pointer receiver for RunCommand(). Then RunCommand() wouldn't modify the global variable.

Rest LGTM.

Done! I'll merge it.

#8 Updated by Ward Vandewege 5 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF