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

Flatten FlavorResourceQuantities #2447

Closed
5 tasks done
gabesaba opened this issue Jun 19, 2024 · 0 comments · Fixed by #2721
Closed
5 tasks done

Flatten FlavorResourceQuantities #2447

gabesaba opened this issue Jun 19, 2024 · 0 comments · Fixed by #2721
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@gabesaba
Copy link
Contributor

gabesaba commented Jun 19, 2024

What would you like to be cleaned:
FlavorResourceQuantities, a map[string]map[string]int64, is ubiquitous in the codebase. We have a lot of code that processes it in a nested loop, often allocating the inner map in the middle. It started getting in my way during the implementation of #79, and I started using a flat version of this map, resulting in significantly simpler code.

Unfortunately, this type is everywhere and the refactor is quite involved. I propose the following steps:

  • Move FlavorResourceQuantities to its own package
  • Define new types, a FlavorResource tuple, and FlavorResourceQuantitiesFlat
  • Update tests without changing functionality - we will define test data using the new type, but convert it back to the old type with a helper function
  • Update non-test source to use this new type
  • Cleanup; rename FlavorResourceQuantitiesFlat to FlavorResourceQuantities

The first three parts are safe. The 4th part has more risk, and can be done alongside the implementation of #79.

Why is this needed:
This old type is a mismatch for the problem, and is resulting in unnecessarily nested/complex code.

To help motivate this change, and see why I think the type is a better fit for the problem, consider the following refactors which were possible:

this was reduced to a single for-loop without branching

func updateFlavorUsage(wi *workload.Info, flvUsage FlavorResourceQuantities, m int64) {
for _, ps := range wi.TotalRequests {
for wlRes, wlResFlv := range ps.Flavors {
v, wlResExist := ps.Requests[wlRes]
flv, flvExist := flvUsage[wlResFlv]
if flvExist && wlResExist {
if _, exists := flv[wlRes]; exists {
flv[wlRes] += v * m
}
}
}
}
}

this was reduced to a single for-loop, with only 1 more level of branching (the if statements)

func updateCohortUsage(wi *workload.Info, cq *ClusterQueue, m int64) {
for _, ps := range wi.TotalRequests {
for wlRes, wlResFlv := range ps.Flavors {
v, wlResExist := ps.Requests[wlRes]
flv, flvExist := cq.Cohort.Usage[wlResFlv]
if flvExist && wlResExist {
if _, exists := flv[wlRes]; exists {
after := cq.Usage[wlResFlv][wlRes] - cq.guaranteedQuota(wlResFlv, wlRes)
// rollback update cq.Usage
before := after - v*m
if before > 0 {
flv[wlRes] -= before
}
// simulate updating cq.Usage
if after > 0 {
flv[wlRes] += after
}
}
}
}
}
}

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant