PP-1018: Design document review of Placement set sorting feature

[Shrini-h]

Hi,

This Topic is to discuss and review the design of Placement set sorting feature.

Design document link: https://pbspro.atlassian.net/wiki/spaces/PD/pages/75628548

Jira link: PP-1018

Please have a look and provide your opinions.

Thanks
Shrini

[bhroam]

I have some comments:

• First off, why node_group_sort_key instead of a formula? When we introduced the formula to jobs, it pretty much replaced job_sort_key.
• No need for visibility any more. Remove Public
• Experimental is no longer a valid change control, use Stable instead
• I disagree that node_group_sort_key should be extended to allow string and string array resources. It isn’t that useful and will likely confuse customers. If you sort an alphanumeric sequence, you go from 1 to 11-19 to 2 to 21-29 to 3 to 31-39. Your first example has 1-6, so it doesn’t have this problem. Customers will have more than 6.
  • Also, sorting on a string array makes no sense. It’s not a single value, it’s an array of values.
  • I don’t see sorting on strings as part of the ticket. If this is a requirement, have the PO update user story.
• Ascending order is low to high. Descending is high to low
• I wouldn’t call the change that adds the new placement sets subtle. It’s an RFE by itself. It should get its own ticket with use cases for it. PP-1018 doesn’t talk about this functionality at all. There’s nothing stopping you from doing both together, but let’s keep them separate in terms of tickets and use cases.
  • You will have a difficult time naming these sets. Placement sets aren’t named by the value of the resource. They are named ‘resource=value’ (e.g., color=blue not blue). The naming scheme you came up with won’t work. Unless you mean that R1-S1 will be router=R1-switch=S1. That is getting a little long.
  • typo: elleven=eleven
  • In your ‘**’ you talk about a placement set containing all nodes. There is no such placement set. When the scheduler spans, it will span across all nodes unassociated with any queue.
• In your examples you don’t need to specify the value for do_not_span_psets. It does not affect the creation of placement sets.
• I find example 2 too large and unruly. It’s hard to follow an example with 37 nodes in it.
• Example 2 is misleading. You left off vnode1-9 because they wouldn’t alpha-sort properly.
• Your sorting is incorrect in example 2. rt1 would be followed by all the rt1-s* sets, rt2 would be followed by all of the rt2-s* sets, etc.
• Remove example 3. It’s not filled out.
• There’s a stray comment at the end of the EDD.

[Shrini-h]

Thank you Bhroam for the comments.
Sorry I went a little overboard on generalizing the feature with the given problem

I will remove my imaginary reqs and rework on the interface design for a formula interface.

meanwhile my answers to your comments

@bhroam

First off, why node_group_sort_key instead of a formula? When we introduced the formula to jobs, it pretty much replaced job_sort_key.

I did evaluate this as a formula and found it hard to express the default behavior in formula.
Felt it will be more confusing and doing a formula_evaluate() for sorting is a overhead for this task.
I agree the formula will help in setting it dynamically.
I will try reworking design for formula, please help me with a sample formula example.

I disagree that node_group_sort_key should be extended to allow string and string array resources. It isn’t that useful and will likely confuse customers.

With the current design I felt it was useful and made more easy for customer to prioritize sets of placement sets. This was the crux of my design. Anyways with the new formula design rework, this will be gone.

If you sort an alphanumeric sequence, you go from 1 to 11-19 to 2 to 21-29 to 3 to 31-39. Your first example has 1-6, so it doesn’t have this problem. Customers will have more than 6.

It was just an example. The feature doesn’t mandate on the naming of the psets. The customer can always prefix the smaller digit numbers with zero. i.e we can go from 01 to 39.

I don’t see sorting on strings as part of the ticket. If this is a requirement, have the PO update user story.

As i will rework on the design with formula interface, this requirement will get void

Ascending order is low to high. Descending is high to low

I also have same understanding, but I did a misplaced them.

I wouldn’t call the change that adds the new placement sets subtle. It’s an RFE by itself. It should get its own ticket with use cases for it. PP-1018 doesn’t talk about this functionality at all. There’s nothing stopping you from doing both together, but let’s keep them separate in terms of tickets and use cases.

Yes I agree its a RFE by itself. As I said I will remove this fictitious requirement while reworking the design

You will have a difficult time naming these sets. Placement sets aren’t named by the value of the resource. They are named ‘resource=value’ (e.g., color=blue not blue). The naming scheme you came up with won’t work. Unless you mean that R1-S1 will be router=R1-switch=S1. That is getting a little long.

In your ‘**’ you talk about a placement set containing all nodes. There is no such placement set. When the scheduler spans, it will span across all nodes unassociated with any queue.

Thank you for enlightening me.

In your examples you don’t need to specify the value for do_not_span_psets. It does not affect the creation of placement sets.

I was aware on this, it was just an unrelated scenario.

I find example 2 too large and unruly. It’s hard to follow an example with 37 nodes in it.

I agree. It was hard for me to create it too.

Example 2 is misleading. You left off vnode1-9 because they wouldn’t alpha-sort properly.

I disagree. I first started with vnode0 and then as I added the “router” string array resource I inserted the router index “1” to the vnode number to make it vnode10. I could have easily used vnode00,01 and so on. As I said the design doesn’t mandate the naming.

Your sorting is incorrect in example 2. rt1 would be followed by all the rt1-s* sets, rt2 would be followed by all of the rt2-s* sets, etc.

As per my imagined design it is correct. The idea was to first consider a specific psets and if not available, step back for a more generalized. anyways this will be gone with redesign

Remove example 3. It’s not filled out.
There’s a stray comment at the end of the EDD.

I agree.