Arvados

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

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.

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 developer-run-tests: #2512

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.

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.