Skip to content

Conversation

@Sanskar531
Copy link
Contributor

#17742

Job allocations can now be reverse sorted. I have also updated the docs for it.

@Sanskar531
Copy link
Contributor Author

I am not sure if this is the right way to do it. Other implementations like the one in DeploymentsByIDPrefix , sort it directly when fetching the data from memdb. Doing so, is a big change though as there are 100 references of where AllocsByJob is used and I will have to add a new sort parameter to the function. If it's okay, I don't mind changing this pr to update all references of AllocsByJob.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @Sanskar531! Thanks for this PR! As you've noted in your PR description, we should be pushing the reverse parameter down into the state query. The reason is because of pagination. For example, suppose I have the following allocations:

  • aaaaaaaa-71ba-11ee-950b-bfbf31e28f07
  • bbbbbbbb-71ba-11ee-b9ea-af44d029c7fa
  • cccccccc-71ba-11ee-80f2-4be2f9fab70e
  • dddddddd-71ba-11ee-81a1-db53adff8b79
  • eeeeeeee-71ba-11ee-976a-3f503a3105b0

And then I make a query with per_page=3 and reverse=true. The way you've got it here, we'd return the following:

  • cccccccc-71ba-11ee-80f2-4be2f9fab70e
  • bbbbbbbb-71ba-11ee-b9ea-af44d029c7fa
  • aaaaaaaa-71ba-11ee-950b-bfbf31e28f07

But what we really want to return is:

  • eeeeeeee-71ba-11ee-976a-3f503a3105b0
  • dddddddd-71ba-11ee-81a1-db53adff8b79
  • cccccccc-71ba-11ee-80f2-4be2f9fab70e

Once you've got that done I'll be happy to re-review. Also, can you run make cl to add a changelog entry?

enabled, this value must match a namespace that the token is allowed to
access. This is specified as a query string parameter.

- `reverse` `(bool: false)` - Specifies the list of returned allocations to be sorted in the reverse lexicographical order. By default, they are lexicographically sorted.
Copy link
Member

@tgross tgross Oct 23, 2023

Choose a reason for hiding this comment

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

Added "by ID", and also we hard-wrap markdown text:

Suggested change
- `reverse` `(bool: false)` - Specifies the list of returned allocations to be sorted in the reverse lexicographical order. By default, they are lexicographically sorted.
- `reverse` `(bool: false)` - Specifies the list of returned allocations to be
sorted in the reverse lexicographical order. By default, they are
lexicographically sorted by ID.

@tgross tgross added theme/api HTTP API and SDK issues type/enhancement labels Oct 23, 2023
@Sanskar531
Copy link
Contributor Author

Hey! Thanks for the guidance. That makes a lot of sense!

So just confirming before I rework this that I can make the change to the function signature of AllocsByJob by adding a new reverse parameter and subsequently updating all the usages of it (I found 100 references) right?

@tgross
Copy link
Member

tgross commented Oct 24, 2023

So just confirming before I rework this that I can make the change to the function signature of AllocsByJob by adding a new reverse parameter and subsequently updating all the usages of it (I found 100 references) right?

Yeah, that looks like the right approach. But there's a state.SortOption type you can use for clarity, rather than a simple bool parameter.

@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows stage/waiting-reply theme/api HTTP API and SDK issues type/enhancement

Projects

Status: Needs Roadmapping

Development

Successfully merging this pull request may close these issues.

2 participants