Consider using --concurrency instead of --concurrency-limit

[duglin]

In what area(s)?

Classifications:
/kind good-first-issue
/kind proposal

Describe the feature:

Consider using --concurrency instead of --concurrency-limit to more closely align with the yaml that uses containerConcurrency. The use of the word limit, while technically accurate, is a bit of a deviation from what people would expect if they bounce between the yaml and kn. Plus, it's less typing :-)

This issue isn't too technical, but more of a UX concern.

[cppforlife]

might as well go with --container-concurrency

[duglin]

While it's a bit long, I would prefer that to what we have today since at least it's a direct (word for word) match and doesn't require thinking :-)

[rhuss]

@duglin what would you do then with the --concurrency-target option ? Up to now both are on the same level in the options, which is not so bad IMO. We could think about a --concurrency which then set botj, target and limit to the same value.

wdyt ?

[duglin]

The annotations in question are:

- autoscaling.knative.dev/target
- autoscaling.knative.dev/containerConcurrency

And today these map to kn options as:

--concurrency-target
--concurrency-limit

Based on a slack chat with @vagababov, target is only meaningful when CC is zero, so setting them both to the same value doesn't seem right, except in the zero case.

w.r.t. naming... it's hard. It would be easier if the annotations were more consistent. E.g. if it was containerTarget. "Target" is just such a generic term that using that as-is on the cmd line doesn't seem friendly enough, so prefixing it with concurrency (while inconsistent) is probably the right thing to do.

I think I'm leaning towards:

--concurrency-target
--concurrency

because even though concurrency isn't 100% aligned with the yaml, this does present some consistency between these two options. Going with --container-concurrency, while aligned with the yaml, I think is harder for people to mentally align with --concurrency-target.

[rhuss]

• autoscaling.knative.dev/target
• autoscaling.knative.dev/containerConcurrency

To be precise, for the --concurrency-target you are right, however for --concurrency-limit we are using the CRD field .spec.containerConcurrency, not an annotation.

I don't like to be inconsistent with respect to the concurrency option naming. So I still like the current, symmetric version of --concurrency-limit and --concurrency-target.

[rhuss]

But maybe I don't understand those options properly. My mental picture is of that of a soft (target) and hard (limit) limit. But I might be wrong here, and need to catch up.

[duglin]

Yup it's about soft vs hard limits AFAIK. But the soft limit only applies when cc is zero

[maximilien]

Agree with @rhuss with the beauty and simplicity of concurrency-target and concurrency-limit.

Closing this unless others can chime with better arguments.

[maximilien]

/assign @maximilien

[knative-prow-robot]

@maximilien: GitHub didn't allow me to assign the following users: maxim.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @maxim

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[maximilien]

/close

[knative-prow-robot]

@maximilien: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.