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

fix: historical allocations not appearing #9522

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

eecsliu
Copy link
Contributor

@eecsliu eecsliu commented Jun 13, 2024

Ticket

DET-10311

Description

Historical allocations weren't showing up sometimes because the data was not present. This can occur when we attempt to calculate the historical allocation for the same date again, violating a constraint and failing the whole transaction. This leads to us repeatedly failing to calculate historical allocation and the data remaining empty. To fix this, we add a clause so that nothing happens when a conflict happens.

Test Plan

No manual testing required.

Testing this is difficult to reproduce, but if you would like to verify, one way is (on your own devcluster) go to master/internal/db/postgres_cluster.go line 99 and add lastDatePtr=nil. This forces there to be a key conflict. If devcluster runs successfully, then the PR works.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@eecsliu eecsliu requested a review from a team as a code owner June 13, 2024 20:35
@cla-bot cla-bot bot added the cla-signed label Jun 13, 2024
Copy link

netlify bot commented Jun 13, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 567ac3f
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/666b57feb27acd00088debbd

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.28%. Comparing base (9adc092) to head (567ac3f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9522      +/-   ##
==========================================
- Coverage   49.28%   49.28%   -0.01%     
==========================================
  Files        1242     1242              
  Lines      161442   161442              
  Branches     2867     2867              
==========================================
- Hits        79574    79570       -4     
- Misses      81696    81700       +4     
  Partials      172      172              
Flag Coverage Δ
backend 43.90% <ø> (-0.01%) ⬇️
harness 63.81% <ø> (ø)
web 44.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

Copy link
Contributor

@carolinaecalderon carolinaecalderon left a comment

Choose a reason for hiding this comment

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

Looks good, I guess?
Is the fix not apparent in master/internal/db/postgres_cluster.go? Not dropping columns with conflicts seems more like a hack than a fix, but I don't think I understand the problem fully.
Also, mark this as a "chore" not a "fix" for release party, or explicitly state in the description that no testing is needed.

@eecsliu
Copy link
Contributor Author

eecsliu commented Jun 14, 2024

Yeah we can't drop the columns or update them on conflict because the we don't want data that is already present to be modified. The specific problem is related to timezones and how they might (or might not) be saved and how they can change. We are investigating more and definitely want to fix the behavior inside of postgres_cluster.go, but any solution we implement would need to include ON CONFLICT DO NOTHING anyways to cover the case of a timezone change/key conflict. Otherwise we're just not gonna save any historical allocations.

TLDR this doesn't fix the underlying cause, but it addresses the symptoms + I would expect the historical allocation code to handle key conflicts in this way anyways.

@eecsliu eecsliu merged commit 2bce8b6 into main Jun 14, 2024
90 of 105 checks passed
@eecsliu eecsliu deleted the fix-missing-historical-usage branch June 14, 2024 18:21
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.

2 participants