Skip to content
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

chore: change output format of JSON aggregated resource data #2129

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

dzhu
Copy link
Contributor

@dzhu dzhu commented Mar 26, 2021

Contains #2123; look at the last commit of this PR only.

Description

Update the structure of the JSON returned for the aggregated endpoint to
match the ERD. Rather than a separate entry for each aggregation key, we
have one entry per period with maps containing the individual
subentries.

The CSV output remains the same, since we don't really want to express
structured data like that in CSV.

Test Plan

  • run det res agg with all combinations of --monthly and --json, eyeball results

@@ -1,25 +1,63 @@
WITH const AS (
SELECT
daterange($1 :: date, $2 :: date, '[]') AS period
),
months AS (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after reading a few times, with some parameters, seems this query can produce the other. i don't think it's a big enough deal to change though.

Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

// One instance of slots in the cluster being allocated to a task during a day
// (aggregated).
// One instance of slots in the cluster being allocated to a task during a
// period (aggregated).
message ResourceAllocationAggregatedEntry {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a second to grok this API, probably a lot better for a user I agree, but dang is it a lot funkier on the backend. Immediately think of https://en.wikipedia.org/wiki/Object%E2%80%93relational_impedance_mismatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, yeah; I hadn't heard that term, but that's basically exactly how I felt. I tell myself it's our job to do the funky stuff so everything comes out nice.

@stoksc stoksc assigned dzhu and unassigned stoksc Mar 26, 2021
Update the structure of the JSON returned for the aggregated endpoint to
match the ERD. Rather than a separate entry for each aggregation key, we
have one entry per period with maps containing the individual
subentries.

The CSV output remains the same, since we don't really want to express
structured data like that in CSV.
@dzhu dzhu merged commit ec8f02b into determined-ai:master Mar 29, 2021
@dzhu dzhu deleted the res-format branch March 29, 2021 20:37
@dannysauer dannysauer added this to the 0.14.6 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants