new: STORIF-187 - Global quota usage table created.#13197
new: STORIF-187 - Global quota usage table created.#13197skulpok-akamai merged 4 commits intolinode:developfrom
Conversation
e492df1 to
1c7f2aa
Compare
6c6c081 to
154edf3
Compare
6d4cd7e to
dbcea0e
Compare
67ab331 to
ff1f699
Compare
|
This PR is stale because it has been open 15 days with no activity. Please attend to this PR or it will be closed in 5 days |
ff1f699 to
4d7ce9a
Compare
packages/manager/src/features/Account/Quotas/GlobalQuotasTable/GlobalQuotasTableRow.tsx
Outdated
Show resolved
Hide resolved
packages/api-v4/src/quotas/quotas.ts
Outdated
| setMethod('GET'), | ||
| setXFilter(filter), | ||
| setParams(params), | ||
| ); |
There was a problem hiding this comment.
Do you need a "get all" method as well? Could we have more than 500 records?
There was a problem hiding this comment.
Currently there is only one object in the list and I think there won't be more than 25 global quotas at all.
We ain't gonna need it =)
| const globalQuotaIds = | ||
| globalQuotas?.data.map((quota) => quota.quota_id) ?? []; | ||
| const globalQuotaUsageQueries = useQueries({ | ||
| queries: globalQuotaIds.map((quotaId) => |
There was a problem hiding this comment.
How many quota IDs could you end up having to fetch in parallel? Asking because this could end up being a bad performance bottleneck if running queries for dozens (hundreds) of IDs.
There was a problem hiding this comment.
The decision to fetch quota usage like this was made by the Architect (from what I know), currently there is only one global quota in the object-storage, and I don't think there will ever be more than 25.
5e50de3 to
6e7c8c5
Compare
6e7c8c5 to
c46afb7
Compare
1f66529 to
d08d18a
Compare
| paginationPreferenceKey: string, | ||
| collectionName: string, | ||
| enabled = true | ||
| ) => { | ||
| const pagination = usePaginationV2({ |
There was a problem hiding this comment.
There are few things that could be corrected in the usePaginationV2 configuration (like route should be /quotas, or queryParamsPrefix is missing), but actually I don't think we even need it at all. The purpose of usePaginationV2 is to store the table state in query params, so it can be restored when you refresh the page, paste the URL into a browser or use a bookmark. But it won't work for dimensional quotas - endpoint must be also selected in order to fetch the correct list of quotas and we don't have a mechanism to store it in the query params. Also, there is very slim chance that we ever have over 25 per-dimension or global quotas for a given feature. I think we can safely remove usePaginationV2. To make it work, Table would have to be wrapped in Paginate instead of using PaginationFooter. If it ever happens that there are a lot quotas per dimension, fetching usage would have to be optimized.
@abailly-akamai Do you see any reason to keep usePaginationV2?
| export const useGetObjUsagePerEndpoint = (selectedLocation: string) => { | ||
| export const useGetQuotasWithUsage = ( | ||
| selectedLocation: string, | ||
| selectedService: QuotaType, |
There was a problem hiding this comment.
The service is declarative in the code, not selected by the user (as opposed to location, though in the location case I don't like that the name implies how the "location" was determined). service or quotaService seem to be better names. Also, the QuotaType type now clashes with the quota_type field, which means something completely different, so we may consider renaming the type as well.
There was a problem hiding this comment.
Agree about "QuotaType", It could be renamed into something more description (e.g ServiceType).
About selectedService/Location, I assume there will be added a select component later that will allow users to select between different services, hence the name.
There was a problem hiding this comment.
Or, all service quotas will be displayed on a single page. That's the thing - you shouldn't assume here how the service will be provided, this is not the concern of this hook.
| }); | ||
|
|
||
| const hasSelectedLocation = Boolean(selectedLocation); | ||
| const isLocalQuotaScope = collectionName !== 'global-quotas'; |
There was a problem hiding this comment.
This is incorrect. You cannot determine if the quota is global or dimensional based on the collection name. For example, global quotas may be provided by /quotas, and multiple dimensional quotas may be provided by other paths (for example, quotas-per-region, quotas-per-instance, etc.). "/quotas" is the natural choice for global quotas when you have multiple quota dimensions.
The information whether quotas are per-dimension or global should be passed with a boolean prop.
There was a problem hiding this comment.
Okay, Block Storage quotas are in the design and there are 2 alternative solutions considered - either using single endpoint for both global and per-region quotas, or using /global-quotas for global quotas. If they go with /global-quotas, then indeed /global-quotas could be bound to global quotas - a perfect example of emergent architecture...
If you don't want expose both collectionName and, e.g., isGlobalScope in QuotaTable props, we could expose just isGlobalScope prop (so turn this around), and set collectionName to global-quotas in QuotasTable when isGlobalScope is true. I would still keep the collectionName for useQuotasQuery though to have some flexibility for the future.
| data: quotasWithUsage, | ||
| quotas, | ||
| errorMessage: quotasErrorMessage, | ||
| queries: quotaUsageQueries, |
There was a problem hiding this comment.
It doesn't seem to be used.
There was a problem hiding this comment.
quotaUsageQueries - used inside the QuotasTableRow component to extract errors.
| const SERVICE = 'object-storage'; | ||
|
|
||
| export const useGetObjUsagePerEndpoint = (selectedLocation: string) => { | ||
| export const useGetQuotasWithUsage = ( |
There was a problem hiding this comment.
Maybe simple useGetQuotas? There is no hook version without usage.
There was a problem hiding this comment.
Might be better to hide the complexity and the fact that usage uses a separate endpoint.
| isError: isQuotasError, | ||
| isFetching: isFetchingQuotas, | ||
| } = useQuotasQuery( | ||
| SERVICE, | ||
| selectedService, | ||
| collectionName, | ||
| { | ||
| page: pagination.page, | ||
| page_size: pagination.pageSize, |
There was a problem hiding this comment.
If we drop usePaginationV2 we could simple use max allowed page size (i.e. 500) and use page 1. It's highly unlikely that we ever get more than 500 quotas per dimension per service.
We are filtering the data on the UI side, so using pagination wouldn't work anyway.
There was a problem hiding this comment.
As discussed offline, it's ok to deviate from the standards, and client side sort/filter/paginate your data in this case. I understand we're dealing with a small data set.
The only thing i would do is, even if you don't anticipate more 500 records, we're still fetching paginated data. I would still do a getAll query + hook. If anybody ever comes to your project the intent is much clearer that way.
It will still result in only fetching the first page, but again, showing the intent.
I would also comment the reason for doing so in code.
You can then decide to to client side paginate or no pagination at all, this is more so a UI/UX decision at that point.
| @@ -32,8 +37,6 @@ interface QuotasTableRowProps { | |||
| setSupportModalOpen: (open: boolean) => void; | |||
| } | |||
|
|
|||
| const quotaRowMinHeight = 58; | |||
There was a problem hiding this comment.
I think passing quotaRowMinHeight as a prop instead of externalizing it to utils would be a cleaner approach.
| @@ -56,8 +64,14 @@ export const useGetObjUsagePerEndpoint = (selectedLocation: string) => { | |||
|
|
|||
| return { | |||
| data: quotaWithUsage, | |||
| quotas, | |||
There was a problem hiding this comment.
What's the point of returning quotas if quotaWithUsage (should be quotasWithUsage actually) already contains all the data?
There was a problem hiding this comment.
It is needed for pagination in order to get total item count, I can either expose "totalItems" property instead of "quotas" or remove pagination completely.
4dd09ff to
d4fc327
Compare
| ...quota, | ||
| usage: quotaUsageQueries?.[index]?.data, | ||
| })) ?? [], | ||
| [quotas, quotaUsageQueries] |
There was a problem hiding this comment.
You might wanna look at that Eslint warning...
d4fc327 to
7e2c3ef
Compare
skulpok-akamai
left a comment
There was a problem hiding this comment.
Please add a feature flag for Global Quotas.
7e2c3ef to
0a29c95
Compare
3bde8c0 to
15ee1ad
Compare
Cloud Manager UI test results🎉 866 passing tests on test run #40 ↗︎
|
abailly-akamai
left a comment
There was a problem hiding this comment.
This looks good overall. I am lacking context about the feature & scope, but implementation looks clean and pagination removal is clear.
Approving to unblock, pending addressing some of the existing comments as needed, and validation by the storage team
| selectedLocation: string, | ||
| selectedService: QuotaType, | ||
| collectionName: string, | ||
| enabled = true |
There was a problem hiding this comment.
I would strongly encourage to use named argument VS positional ones. It will make your instances much more readable. Past two arguments, it's usually good DX practice
| service: QuotaType, | ||
| collection: string, | ||
| id: string, | ||
| enabled = true, |
There was a problem hiding this comment.
same comment for the arguments in those queries. not blocking but improving DX can go a long way 🤷
Description 📝
Global quota usage table created.
Changes 🔄
useGetObjGlobalQuotasWithUsagehook.GlobalQuotasTablecomponent.Preview 📷
How to test 🧪
docker-compose up -d apinext..envfile of the Cloud Manager to use local API instance.pnpm dev/quotaspage.Related PRs
apinext
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅