Chia-Ping Tsai created YUNIKORN-2268:
----------------------------------------
Summary: property key should be treated as case-sensitive
Key: YUNIKORN-2268
URL: https://issues.apache.org/jira/browse/YUNIKORN-2268
Project: Apache YuniKorn
Issue Type: Improvement
Reporter: Chia-Ping Tsai
Assignee: Dong-Lin Hsieh
*Summary:*
# document the rules (site)
# add check for property key (core)
# add release entry (release)
*Discussion*
*Chia-Ping*
{quote}
dear all,there are 2 questions (or ideas) about configs' properties
1. should we validate the property (key)? it can produce fast-fail but the
behavior get changed.
2. should property value be case insensitive? it seems only value of
{{application.sort.policy}} is case sensitive
thanks, and please feel free to correct me :)
{quote}
*Craig*
{quote}
We explicitly do not validate properties because they are meant to be
extensible. For example, there are numerous logging settings for both the shim
and core, and neither side knows which are allowed y the other. This goes for
all properties, not just those for logging. We configure the admission
controller, shim, and core from the same configuration, so we don’t want to
reject unknown properties.
Case-sensitivity is a thorny subject. There’s two things — the keys and the
values. Values absolutely need to be treated as case-sensitive in general
because it’s not always the case that two values differing only in case
represent the same thing. File systems for example may be case-sensitive or
not, and assuming that “FILE” == “file” is dangerous. There is also language
considerations. Depending on locale, case equivalence rules can differ at
runtime. This has in many pieces of software historically led to a wide variety
of bugs that are difficult to diagnose. IMO, configuration should always be
case-sensitive to avoid these issues. In the case of YuniKorn we have some
historical baggage around some property names being declared case-insensitive,
and it would be a breaking change to make them case-sensitive moving forward,
so that’s something we will have to live with.
It may seem like converting existing properties to be case-insensitive would be
safe, but this can also break in subtle ways. Previously functional
configurations that had mismatched property names might now result in different
actions. For example:
A: true
a: false
If ‘a’ is case-sensitive it’s clear which value gets used. If this property is
now changed to be case-insensitive, what value will the property contain? What
if these entries are located in two different configmaps (yunikorn-defaults and
yunikorn-configs)?
TLDR; existing semantics should be preserved to avoid backwards-compatibility
problems, and any future properties should be treated as case-sensitive to
avoid locale-related bugs.
{quote}
*Chia-Ping*
{quote}
the "supported values" from docs only show the lower case values ... so maybe
we can fix it directly and don't worry about BC ?
https://yunikorn.apache.org/docs/user_guide/queue_config#preemptionpolicy
{quote}
*Craig*
{quote}
If we do change those to case sensitive, it’s probably best to do it gradually
by allowing wrong case but warning for a release or two, and then forcing
lowercase later. That would at least warn users who are upgrading that their
configurations need fixing.
And of course this definitely would need a release notes entry.
Even in this case I would only change the handling of the key names, not the
property values. So for example, application.sorting.policy can be “fifo” or
“FIFO” today and that’s ok. I wouldn’t change that since it’s been documented
to work that way.
Same goes for things like log levels, etc. those are parsed by a 3rd party
library so we accept what they do.
{quote}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@yunikorn.apache.org
For additional commands, e-mail: dev-h...@yunikorn.apache.org
