Thanks a lot for writing this summary Ufuk! > Do you agree that approach 1 has been rejected? I don't think so. To me, using conversion is prone to errors to the same degree as escaping in option 2 (plus inconvenience in editing).
> Is there any objection to approach 2? Not from my side. > Did we consider whether approach 3 is good enough? Does this mean putting all system properties into the Configuration eagerly? > Is approach 4 a viable option after all? I think we should continue the discussion around the questions that were brought up. I think it's still viable. Regards, Roman On Wed, Jan 27, 2021 at 11:50 AM Ufuk Celebi <u...@apache.org> wrote: > Let me try to summarize the current state of the discussion. I think we > now have a few competing approaches. > > # Approach 1: dynamic env var mapping > > * This is the approach currently outlined in the FLIP > * The assumption that we only have dots and hyphens in config option keys > seem to be wrong, e.g. custom backends or metrics reporters (see Chesnay's > responses) > > Due to the 2nd point, this option seems to be not viable. We could opt to > only support the subset of keys that satisfy the restriction on their key, > but this could be confusing to users. > > I'm reading our discussion as rejecting this approach due to its > limitations. > > # Approach 2: single DYNAMIC_PROPERTIES env var > > * Parse DYNAMIC_PROPERTIES env var during configuration load time > * The provided value may include multiple config option entries, similar > to our docker-entrypoint script ( > https://github.com/apache/flink-docker/blob/master/1.12/scala_2.12-java8-debian/docker-entrypoint.sh#L79-L91 > ) > > This option would essentially canonicalize our existing docker-entrypoint > script solution (which does not suffer from any limitations on the keys). > > I think everybody in the discussion agrees that configuration of multiple > options as a single value is cumbersome, but besides that everybody seems > to be generally on board with this approach. > > # Approach 3: main args-based dynamic properties (do nothing) > > * No change required > * Map env vars to config options via command line args dynamic properties, > e.g. -Dkey=$(VALUE) > > I think this is the approach that Ingo now favours. The main question > seems to be whether this is good enough and whether it works in all > contexts we care about. > > # Approach 4: substitute flink-conf.yaml values only > > * flink-conf.yaml entries can reference env vars as values > > This approach was rejected in initial DISCUSS thread, but there seems to > be some unanswered questions here. > > Roman brought up the question of providing a default mapping for all known > keys which would allow individual env vars to configure most options out of > the box. If an option is not part of the default mapping, users would be > able to still define the mapping manually. The main down side seems to be > that we would have to maintain the default mapping somehow. This is > something we had rejected earlier as too fragile. > > # Summary > > A few questions in order to move this thread forward: > > * Do you agree that approach 1 has been rejected? > * Is there any objection to approach 2? > * Did we consider whether approach 3 is good enough? > * Is approach 4 a viable option after all? I think we should continue the > discussion around the questions that were brought up. > > I hope that we can reach a conclusion soon. > > – Ufuk > > On Wed, Jan 27, 2021, at 10:45 AM, Khachatryan Roman wrote: > > Hi Ingo, > > I missed that part of the discussion, sorry about that. > > Would you mind putting it to the FLIP page? > > > > I guess you are referring to this message: > > > I don't think we can assume that we know all config option keys. For > > instance, I might write a custom high availability service or a custom > > FileSystem plugin that has it's own config options. It would be a pity > (but > > maybe tolerable) if env var config would only work with Flink's core > > options. > > Can it be solved by adding an annotating and then scanning classpath via > > reflection? > > This is not ideal, but knowing all options in advance might be a right > step > > towards eager config evaluation. > > > > @Chesnay and Thomas > > Thanks for the clarification. IIUC, the format would be > > DYNAMIC_PROPERTIES='-Drest.port=1234 -Dother.option="iContainAn=Sign"'. > > But the evaluation WILL have nothing to do with System Properties as it > > will happen already inside the java process, right? > > This may be a bit confusing for users (but probably not a big issue > though). > > > > As for the substitution in file, Yang wrote: > > > It is true that we need to maintain a flink configuration file which > > simply maps all keys to some environment variables. But > > > for Yarn and K8s deployment(both standalone on K8s and native), we > > already have such a file, which is shipped from client > > > or mounted from a ConfigMap. > > Can we have a default file in Flink on the classpath with keys already > > mapped to env vars? (as I wrote in the very beginning). > > > > Regards, > > Roman > > > > > > On Wed, Jan 27, 2021 at 9:24 AM Ingo Bürk <i...@ververica.com> wrote: > > > > > Hi Yang, > > > > > > yeah, this is essentially the solution we also use in Ververica > Platform > > > (except that we use ${…}). I can't really add anything other than the > > > previously stated reasons for why we decided to reject this, but of > course > > > I'm also open to this solution still. I don't think Flink necessarily > needs > > > to maintain a flink-conf.yaml with all environment variables; I would > > > assume users generally would want to override only some properties, not > > > most of them, and for a config like that basically any EV not set > would end > > > up causing an error. > > > > > > > > > Regards > > > Ingo > > > > > > On Wed, Jan 27, 2021 at 4:42 AM Yang Wang <danrtsey...@gmail.com> > wrote: > > > > > >> Hi all, sorry to butt in. > > >> > > >> I am little curious about why do we have to do the overwritten via > > >> environment variables after > > >> loading the configuration from file. Could we support to do the > > >> substitution while loading the > > >> "flink-conf.yaml" file? > > >> > > >> For example, I have the following config options in my > flink-conf.yaml. > > >> fs.oss.accessKeyId: $(FS_OSS_ACCESS_KEY_ID) > > >> fs.oss.accessKeySecret: $(FS_OSS_ACCESS_KEY_SECRET) > > >> > > >> I expect these environment variables could be substituted when > loading the > > >> configuration file. It is > > >> very similar to use "*envsubst < /tmp/flink-conf.yaml > > > >> /tmp/flink-conf-1.yaml*". > > >> > > >> I know this is a rejected alternative. But I think some reasons could > not > > >> stand on. > > >> * We could use $(FS_OSS_ACCESS_KEY_ID) instead of > ${FS_OSS_ACCESS_KEY_ID} > > >> for the environment definition > > >> to avoid escape issues. I think the Kubernetes has the same > behavior[1]. > > >> Maybe many users are already familiar with it. > > >> * Users do not need to know the conversion between flink config > option and > > >> environment name. Because they could use > > >> any name they want. > > >> * It is true that we need to maintain a flink configuration file > > >> which simply maps all keys to some environment variables. But > > >> for Yarn and K8s deployment(both standalone on K8s and native), we > already > > >> have such a file, which is shipped from client > > >> or mounted from a ConfigMap. > > >> > > >> > > >> @Ingo This solution could perfectly work with Kubernetes deployment > and is > > >> easier to use. We could use a ConfigMap for > > >> storing the flink-conf.yaml, and using secrets to exposed as > environment > > >> variables for the authentication informations. > > >> > > >> > > >> [1]. > > >> > > >> > https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/#using-environment-variables-inside-of-your-config > > >> > > >> > > >> Best, > > >> Yang > > >> > > >> Chesnay Schepler <ches...@apache.org> 于2021年1月27日周三 上午8:03写道： > > >> > > >> > In the end DYNAMIC_PROPERTIES behaves exactly like env.java.opts; > > >> > meaning that the existing rules set by the JVM apply. > > >> > > > >> > Here's an example: export DYNAMIC_PROPERTIES='-Drest.port=1234 > > >> > -Dother.option="iContainAn=Sign"' > > >> > > > >> > This would then be appended as is to the /java/ command. > > >> > ( > > >> > Conceptually at least; shells are annoying when it comes to > > >> > quotes/whitespace; good old http://mywiki.wooledge.org/BashFAQ/050 > . > > >> > Something like java ... $(eval echo ${DYNAMIC_PROPERTIES} > handles > > >> > quotes nicely, but no spaces in values. > > >> > > > >> > We should really move more logic from the scripts into the > > >> > BashJavaUtils... > > >> > ) > > >> > > > >> > On 1/26/2021 11:17 PM, Khachatryan Roman wrote: > > >> > >> Here's an example: My option key is custom.my_backend_option. > With > > >> the > > >> > >> current design, the corresponding env variable would be > > >> > >> CUSTOM_MY_BACKEND_OPTION, which would be converted into > > >> > >> custom.my.backend.option . > > >> > > I think we don't have to translate CUSTOM_MY_BACKEND_OPTION back. > > >> > Instead, > > >> > > we should use the key from the ConfigOption. > > >> > > I'm assuming that not every ENV VAR will end up in the > Configuration > > >> - > > >> > > only those for which a matching ConfigOptions is found. > > >> > > > > >> > > I'm also fine with a single ENV VAR (DYNAMIC_PROPERTIES). It's > > >> already a > > >> > > big improvement. > > >> > > In the future, we can consider adding smth like > > >> ConfigOption.withEnvVar > > >> > for > > >> > > some (most popular) options. > > >> > > > > >> > > However, escaping is still not clear to me: how would kv-pairs be > > >> > > separated? What if such a separator is in the value itself? What > if > > >> '=' > > >> > is > > >> > > in the value? > > >> > > Or am I missing something? > > >> > > > > >> > > Regards, > > >> > > Roman > > >> > > > > >> > > > > >> > > On Tue, Jan 26, 2021 at 6:41 PM Till Rohrmann < > trohrm...@apache.org> > > >> > wrote: > > >> > > > > >> > >> Thinking a bit more about the DYNAMIC_PROPERTIES, I have to admit > > >> that I > > >> > >> like the fact that it works around the problem of encoding the > key > > >> names > > >> > >> and that it is more powerful wrt to bulk changes. Also the fact > that > > >> one > > >> > >> can copy past configuration snippets is quite useful. Given these > > >> > aspects > > >> > >> and that we wouldn't exclude any mentioned configuration > scenarios, I > > >> > would > > >> > >> also be ok following this approach given that we support it for > all > > >> > Flink > > >> > >> processes. > > >> > >> > > >> > >> Cheers, > > >> > >> Till > > >> > >> > > >> > >> On Tue, Jan 26, 2021 at 5:10 PM Ingo Bürk <i...@ververica.com> > > >> wrote: > > >> > >> > > >> > >>> Hi everyone, > > >> > >>> > > >> > >>> thanks for the livid discussion, it's great to see so many > opinions > > >> and > > >> > >>> ideas! > > >> > >>> > > >> > >>> The point about underscores is a very valid point where the > current > > >> > FLIP, > > >> > >>> if we were to stick with it, would have to be improved. I was > going > > >> to > > >> > say > > >> > >>> that we should exclude that from the discussion about the > merits of > > >> > >>> different overall solutions, but I am afraid that this makes the > > >> "how > > >> > to > > >> > >>> name EVs" question even more convoluted, and that in turn > directly > > >> > impacts > > >> > >>> the usefulness of the FLIP as a whole which is about a more > > >> convenient > > >> > way > > >> > >>> of configuring Flink; names which are too cryptic will not > achieve > > >> > that. > > >> > >>> So > > >> > >>> in this regard I am in agreement with Chesnay. > > >> > >>> > > >> > >>> After all these considerations, speaking from the Kubernetes > > >> context, > > >> > it > > >> > >>> seems to me that using the dynamic properties works best (I can > use > > >> > config > > >> > >>> key names as-is) and requires no change, so I'm actually just > > >> leaning > > >> > >>> towards that. However, the Kubernetes context is, I guess, not > the > > >> only > > >> > >>> one > > >> > >>> to consider. > > >> > >>> > > >> > >>> > > >> > >>> Best regards > > >> > >>> Ingo > > >> > >>> > > >> > >>> On Tue, Jan 26, 2021 at 3:48 PM Chesnay Schepler < > > >> ches...@apache.org> > > >> > >>> wrote: > > >> > >>> > > >> > >>>> Mind you that we could of course solve these character issues > by > > >> first > > >> > >>>> nailing down which characters we allow in keys (presumably: > > >> > [a-z0-9-.]). > > >> > >>>> > > >> > >>>> On 1/26/2021 3:45 PM, Chesnay Schepler wrote: > > >> > >>>>> Here's an example: My option key is custom.my_backend_option. > With > > >> > the > > >> > >>>>> current design, the corresponding env variable would be > > >> > >>>>> CUSTOM_MY_BACKEND_OPTION, which would be converted into > > >> > >>>>> custom.my.backend.option . > > >> > >>>>> > > >> > >>>>> It is true that users could still parse the original system > > >> property > > >> > >>>>> as a fall-back, but it seems to partially invalidate the goal > and > > >> > >>>>> introduce the potential for surprises and inconsistent > behavior. > > >> > >>>>> > > >> > >>>>> What would happen if the option were already defined in the > > >> > >>>>> flink-conf.yaml, but overwritten with the env variable? Users > > >> would > > >> > >>>>> have to check every time they access a configuration whether > the > > >> > >>>>> system property was also set and resolve things manually. > > >> Naturally > > >> > >>>>> things might also conflict with whatever prioritization we > come up > > >> > >>> with. > > >> > >>>>> Now you might say that this is only necessary if the option > > >> contains > > >> > >>>>> special characters, but then we're setting users up for a > surprise > > >> > >>>>> should they ever rename an existing option to contain an > > >> underscore. > > >> > >>>>> > > >> > >>>>> As for newlines, I wouldn't expect newline characters to > appear > > >> > within > > >> > >>>>> DYNAMIC_VARIABLE, but I guess it would follow the same > behavior > > >> as if > > >> > >>>>> you would declare them on the command-line? > > >> > >>>>> > > >> > >>>>> One more downside I see is that from a users perspective I'd > > >> always > > >> > >>>>> have to do this conversion manually. You can't just copy stuff > > >> from > > >> > >>>>> the documentation (unless we duplicate every single mention), > nor > > >> can > > >> > >>>>> you easily switch between environment variables and dynamic > > >> > >>>>> properties/flink-conf.yaml . For the use-cases that people > seems > > >> to > > >> > be > > >> > >>>>> concerned about (where you have lots of variables) I would > think > > >> that > > >> > >>>>> this is a deal-breaker. > > >> > >>>>> > > >> > >>>>> On 1/26/2021 2:59 PM, Khachatryan Roman wrote: > > >> > >>>>>> @Chesnay > > >> > >>>>>> could you explain how underscores in user-defined properties > > >> would > > >> > be > > >> > >>>>>> affected with transformation like STATE_BACKEND -> > state.backend? > > >> > >>>>>> IIUC, this transformation happens in Flink and doesn't alter > ENV > > >> > >>>>>> vars, so > > >> > >>>>>> the user app can still parse the original configuration. > > >> > >>>>>> OTH, I'm a bit concerned whether the newline should be > escaped by > > >> > the > > >> > >>>>>> user > > >> > >>>>>> in DYNAMIC_VARIABLES. > > >> > >>>>>> > > >> > >>>>>> @Ingo Bürk <i...@ververica.com> > > >> > >>>>>> I feel a bit lost in the discussion) Maybe we can put an > > >> > intermediate > > >> > >>>>>> summary of pros and cons of different approaches into the > FLIP? > > >> > >>>>>> > > >> > >>>>>> And for completeness, we could combine DYNAMIC_VARIABLES > approach > > >> > >>> with > > >> > >>>>>> passing individual variables. > > >> > >>>>>> > > >> > >>>>>> > > >> > >>>>>> Regards, > > >> > >>>>>> Roman > > >> > >>>>>> > > >> > >>>>>> > > >> > >>>>>> On Tue, Jan 26, 2021 at 12:54 PM Chesnay Schepler < > > >> > >>> ches...@apache.org> > > >> > >>>>>> wrote: > > >> > >>>>>> > > >> > >>>>>>> I think we have to assume that some user has a custom config > > >> option > > >> > >>>>>>> that > > >> > >>>>>>> uses underscores. > > >> > >>>>>>> > > >> > >>>>>>> That said, we can probably assume that no one uses other > special > > >> > >>>>>>> characters like question marks and such, which are indeed > > >> allowed > > >> > by > > >> > >>>>>>> the > > >> > >>>>>>> YAML spec. > > >> > >>>>>>> > > >> > >>>>>>> These kind of questions are precisely why I prefer the > > >> > >>>>>>> DYNAMIC_VARIABLES > > >> > >>>>>>> approach; you don't even have to worry about this stuff. > > >> > >>>>>>> The only question we'd have to answer is whether manually > > >> defined > > >> > >>>>>>> properties should take precedent or not. > > >> > >>>>>>> > > >> > >>>>>>> @Uce I can see how it could be cumbersome to modify, but at > the > > >> > same > > >> > >>>>>>> time you can implement whatever other approach you want on > top > > >> of > > >> > >>> it: > > >> > >>>>>>> // this is a /conceptual /example for an optional setting > > >> > >>>>>>> DYNAMIC_VARIABLES="${REST_PORT_SETTING}" > > >> > >>>>>>> if _someCondition_: > > >> > >>>>>>> export REST_PORT_SETTING="-Drest.port=1234" > > >> > >>>>>>> > > >> > >>>>>>> // this is a /conceptual /example for a configurable setting > > >> > >>>>>>> DYNAMIC_VARIABLES="-Drest.port=${MY_FANCY_VARIABLE:-8200}" > > >> > >>>>>>> if _someCondition_: > > >> > >>>>>>> export MY_FANCY_VARIABLE="1234" > > >> > >>>>>>> > > >> > >>>>>>> Additionally, this makes it quite easy to audit stuff, > since we > > >> can > > >> > >>>>>>> just > > >> > >>>>>>> eagerly log what DYNAMIC_VARIABLES is set to. > > >> > >>>>>>> > > >> > >>>>>>> On 1/26/2021 12:48 PM, Xintong Song wrote: > > >> > >>>>>>>> @Ufuk, > > >> > >>>>>>>> I also don't find any existing options with underscores in > > >> their > > >> > >>> keys. > > >> > >>>>>>>> However, I do not find any explicit rules forbid using them > > >> > either. > > >> > >>>>>>>> I'm > > >> > >>>>>>> not > > >> > >>>>>>>> saying this should block the FLIP. Just it would be nice to > > >> beware > > >> > >>> of > > >> > >>>>>>> this > > >> > >>>>>>>> issue, and maybe ensure the assumption with test cases if > we > > >> > >>> finally > > >> > >>>>>>> decide > > >> > >>>>>>>> to go with these mapping rules. > > >> > >>>>>>>> > > >> > >>>>>>>> Thank you~ > > >> > >>>>>>>> > > >> > >>>>>>>> Xintong Song > > >> > >>>>>>>> > > >> > >>>>>>>> > > >> > >>>>>>>> > > >> > >>>>>>>> On Tue, Jan 26, 2021 at 7:27 PM Ufuk Celebi < > u...@apache.org> > > >> > >>> wrote: > > >> > >>>>>>>>> @Xingtong: The assumption for the mapping was that we only > > >> have > > >> > >>>>>>>>> dots and > > >> > >>>>>>>>> hyphens in the keys. Do you have an example for a key > which > > >> > >>> include > > >> > >>>>>>>>> underscores? If underscores are common for keys (I > couldn't > > >> find > > >> > >>> any > > >> > >>>>>>>>> existing options that use it), it would certainly be a > blocker > > >> > for > > >> > >>>>>>>>> the > > >> > >>>>>>>>> discussed approach. > > >> > >>>>>>>>> > > >> > >>>>>>>>> On Tue, Jan 26, 2021, at 11:46 AM, Xintong Song wrote: > > >> > >>>>>>>>>> - The naming conversions proposed in the FLIP > seems not > > >> > >>>>>>>>>> bijective > > >> > >>>>>>> to > > >> > >>>>>>>>> me. > > >> > >>>>>>>>>> There could be problems if the configuration key > > >> contains > > >> > >>>>>>> underscores. > > >> > >>>>>>>>>> - a_b -> FLINK_CONFIG_a_b > > >> > >>>>>>>>>> - FLINK_CONFIG_a_b -> a.b > > >> > >>>>>>> > > >> > >>>> > > >> > > > >> > > > >> > > > > > >
