FLIP-81: Executor-related new ConfigOptions

[Kostas Kloudas-5]

Hi all,

As part of FLIP-73 (the Executors effort), we would like to introduce
some new configuration options. These are needed in order to be able
to map all the options that the user can specify either
programmatically or through the CLI into configuration options.

The bylaws specify that every user-facing change should pass through a
FLIP, so the FLIP with more details on the new options can be found
here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=133631524

Please keep the discussion in the mailing list.

Thanks,
Kostas

[Aljoscha Krettek-2]

I think this is good to go to a VOTE. We should, however, make it more prominent that we don’t like the “attached”, “shutdown-on-attached-exit”, and “dynamic-properties” and that we only add them for backwards compatibility.

Best,
Aljoscha

> On 21. Oct 2019, at 17:48, Kostas Kloudas <[hidden email]> wrote:
>
> Hi all,
>
> As part of FLIP-73 (the Executors effort), we would like to introduce
> some new configuration options. These are needed in order to be able
> to map all the options that the user can specify either
> programmatically or through the CLI into configuration options.
>
> The bylaws specify that every user-facing change should pass through a
> FLIP, so the FLIP with more details on the new options can be found
> here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=133631524
>
> Please keep the discussion in the mailing list.
>
> Thanks,
> Kostas

[dwysakowicz]

Hi,

I really like the idea of unifying all the ways to set a configuration
option and its keys. I think it nicely aligns with FLIP-59.

I have two comments though.

1. I wonder what are you thoughts on reusing the "parallelism.default"
(CoreOptions.DEFAULT_PARALLELISM) instead of introducing a new key
"execution.parallelism". I am not sure if we should introduce another
key as it might be confusing for users to have two separate config
options for client and cluster side.

Moreover if we go with a new config option for the client side with a
default value of 1, it makes the cluster side option unusable. Right now
the logic is that if we set the parallelism to -1(the default value) on
the client side, the cluster side configuration is used.

Would be interested in an opinion of others on that matter. I think we
have few other cases of options that we set on the client side with a
fallback on the cluster side. I think we should have a common approach
to such scenarios.

2. If I understood it correctly the "dynamic-properties" (maybe also the
"attached" and "shutdown-on-exit") are an internal options that should
never be set manually by the user through the config file. I think they
are internal and should not be exposed. I wonder if we can make them
less visible somehow. Also I think we do not need to agree on them
through the FLIP process.

Best,

Dawid

On 22/10/2019 10:05, Aljoscha Krettek wrote:

> I think this is good to go to a VOTE. We should, however, make it more prominent that we don’t like the “attached”, “shutdown-on-attached-exit”, and “dynamic-properties” and that we only add them for backwards compatibility.
>
> Best,
> Aljoscha
>
>> On 21. Oct 2019, at 17:48, Kostas Kloudas <[hidden email]> wrote:
>>
>> Hi all,
>>
>> As part of FLIP-73 (the Executors effort), we would like to introduce
>> some new configuration options. These are needed in order to be able
>> to map all the options that the user can specify either
>> programmatically or through the CLI into configuration options.
>>
>> The bylaws specify that every user-facing change should pass through a
>> FLIP, so the FLIP with more details on the new options can be found
>> here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=133631524
>>
>> Please keep the discussion in the mailing list.
>>
>> Thanks,
>> Kostas

[Kostas Kloudas-4]

Hi Dawid,

For the first comment, I am also up for re-using as many options as
possible. My only concern though is that so far the -p and the
parallelism.default were considered "different" and I cannot tell the
exact reason. In addition, the "parallelism.default" is not the most
descriptive name in my opinion. So just for caution, I would go with
introducing a new option and if we all agree, I would actually
deprecate the old one, as that is the one with the problematic name.

For the second comment, you are right for the dynamic properties and I
will remove them from the document. For the rest (i.e. the "shutdown
after exit" and the "attached"), I do not think we can remove them as
the user should be allowed to specify them in the configuration.

I am hoping to have more opinions on this, and I will open a voting
thread as soon as we reach a consensus.
Cheers,
Kostas

On Tue, Oct 22, 2019 at 10:49 AM Dawid Wysakowicz
<[hidden email]> wrote:

>
> Hi,
>
> I really like the idea of unifying all the ways to set a configuration
> option and its keys. I think it nicely aligns with FLIP-59.
>
> I have two comments though.
>
> 1. I wonder what are you thoughts on reusing the "parallelism.default"
> (CoreOptions.DEFAULT_PARALLELISM) instead of introducing a new key
> "execution.parallelism". I am not sure if we should introduce another
> key as it might be confusing for users to have two separate config
> options for client and cluster side.
>
> Moreover if we go with a new config option for the client side with a
> default value of 1, it makes the cluster side option unusable. Right now
> the logic is that if we set the parallelism to -1(the default value) on
> the client side, the cluster side configuration is used.
>
> Would be interested in an opinion of others on that matter. I think we
> have few other cases of options that we set on the client side with a
> fallback on the cluster side. I think we should have a common approach
> to such scenarios.
>
> 2. If I understood it correctly the "dynamic-properties" (maybe also the
> "attached" and "shutdown-on-exit") are an internal options that should
> never be set manually by the user through the config file. I think they
> are internal and should not be exposed. I wonder if we can make them
> less visible somehow. Also I think we do not need to agree on them
> through the FLIP process.
>
> Best,
>
> Dawid
>
> On 22/10/2019 10:05, Aljoscha Krettek wrote:
> > I think this is good to go to a VOTE. We should, however, make it more prominent that we don’t like the “attached”, “shutdown-on-attached-exit”, and “dynamic-properties” and that we only add them for backwards compatibility.
> >
> > Best,
> > Aljoscha
> >
> >> On 21. Oct 2019, at 17:48, Kostas Kloudas <[hidden email]> wrote:
> >>
> >> Hi all,
> >>
> >> As part of FLIP-73 (the Executors effort), we would like to introduce
> >> some new configuration options. These are needed in order to be able
> >> to map all the options that the user can specify either
> >> programmatically or through the CLI into configuration options.
> >>
> >> The bylaws specify that every user-facing change should pass through a
> >> FLIP, so the FLIP with more details on the new options can be found
> >> here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=133631524
> >>
> >> Please keep the discussion in the mailing list.
> >>
> >> Thanks,
> >> Kostas
>

[Aljoscha Krettek-2]

Re 1. I think the parallelism setting doesn’t to what was originally planned or expected anymore. As far as I can see it’s only used on the client side. There we only check if the user gave a parallelism using “-p”, if they didn’t we set what is configured under parallelism.default. I’m fine with either reusing the existing setting or adding a new one, but I gravitate towards adding a new one that has a better name (possibly, with specifying parallelism.default as the “deprecated key” for that).

Re 2. The “encoded” properties should be removed. I’m afraid we have to keep “attached” and “sae” for now in order to not break existing client behaviour. But we could also make them hidden/internal options. I’m torn on this one and gravitate towards keeping them as config values but documenting them as experimental and clearly describing what they do and imply.

Best,
Aljoscha

> On 22. Oct 2019, at 14:26, Kostas Kloudas <[hidden email]> wrote:
>
> Hi Dawid,
>
> For the first comment, I am also up for re-using as many options as
> possible. My only concern though is that so far the -p and the
> parallelism.default were considered "different" and I cannot tell the
> exact reason. In addition, the "parallelism.default" is not the most
> descriptive name in my opinion. So just for caution, I would go with
> introducing a new option and if we all agree, I would actually
> deprecate the old one, as that is the one with the problematic name.
>
> For the second comment, you are right for the dynamic properties and I
> will remove them from the document. For the rest (i.e. the "shutdown
> after exit" and the "attached"), I do not think we can remove them as
> the user should be allowed to specify them in the configuration.
>
> I am hoping to have more opinions on this, and I will open a voting
> thread as soon as we reach a consensus.
> Cheers,
> Kostas
>
> On Tue, Oct 22, 2019 at 10:49 AM Dawid Wysakowicz
> <[hidden email]> wrote:
>>
>> Hi,
>>
>> I really like the idea of unifying all the ways to set a configuration
>> option and its keys. I think it nicely aligns with FLIP-59.
>>
>> I have two comments though.
>>
>> 1. I wonder what are you thoughts on reusing the "parallelism.default"
>> (CoreOptions.DEFAULT_PARALLELISM) instead of introducing a new key
>> "execution.parallelism". I am not sure if we should introduce another
>> key as it might be confusing for users to have two separate config
>> options for client and cluster side.
>>
>> Moreover if we go with a new config option for the client side with a
>> default value of 1, it makes the cluster side option unusable. Right now
>> the logic is that if we set the parallelism to -1(the default value) on
>> the client side, the cluster side configuration is used.
>>
>> Would be interested in an opinion of others on that matter. I think we
>> have few other cases of options that we set on the client side with a
>> fallback on the cluster side. I think we should have a common approach
>> to such scenarios.
>>
>> 2. If I understood it correctly the "dynamic-properties" (maybe also the
>> "attached" and "shutdown-on-exit") are an internal options that should
>> never be set manually by the user through the config file. I think they
>> are internal and should not be exposed. I wonder if we can make them
>> less visible somehow. Also I think we do not need to agree on them
>> through the FLIP process.
>>
>> Best,
>>
>> Dawid
>>
>> On 22/10/2019 10:05, Aljoscha Krettek wrote:
>>> I think this is good to go to a VOTE. We should, however, make it more prominent that we don’t like the “attached”, “shutdown-on-attached-exit”, and “dynamic-properties” and that we only add them for backwards compatibility.
>>>
>>> Best,
>>> Aljoscha
>>>
>>>> On 21. Oct 2019, at 17:48, Kostas Kloudas <[hidden email]> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> As part of FLIP-73 (the Executors effort), we would like to introduce
>>>> some new configuration options. These are needed in order to be able
>>>> to map all the options that the user can specify either
>>>> programmatically or through the CLI into configuration options.
>>>>
>>>> The bylaws specify that every user-facing change should pass through a
>>>> FLIP, so the FLIP with more details on the new options can be found
>>>> here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=133631524
>>>>
>>>> Please keep the discussion in the mailing list.
>>>>
>>>> Thanks,
>>>> Kostas
>>

[Kostas Kloudas-4]

Hi all,

Thanks for the discussion!

I will update the FLIP to reflect:
1) I will reuse the core option parallelism.default for the parallelism, and
2) I will remove the dynamic properties from the options.

And I will open a voting thread.

Cheers,
Kostas

On Tue, Oct 22, 2019 at 3:12 PM Aljoscha Krettek <[hidden email]> wrote:

>
> Re 1. I think the parallelism setting doesn’t to what was originally planned or expected anymore. As far as I can see it’s only used on the client side. There we only check if the user gave a parallelism using “-p”, if they didn’t we set what is configured under parallelism.default. I’m fine with either reusing the existing setting or adding a new one, but I gravitate towards adding a new one that has a better name (possibly, with specifying parallelism.default as the “deprecated key” for that).
>
> Re 2. The “encoded” properties should be removed. I’m afraid we have to keep “attached” and “sae” for now in order to not break existing client behaviour. But we could also make them hidden/internal options. I’m torn on this one and gravitate towards keeping them as config values but documenting them as experimental and clearly describing what they do and imply.
>
> Best,
> Aljoscha
>
> > On 22. Oct 2019, at 14:26, Kostas Kloudas <[hidden email]> wrote:
> >
> > Hi Dawid,
> >
> > For the first comment, I am also up for re-using as many options as
> > possible. My only concern though is that so far the -p and the
> > parallelism.default were considered "different" and I cannot tell the
> > exact reason. In addition, the "parallelism.default" is not the most
> > descriptive name in my opinion. So just for caution, I would go with
> > introducing a new option and if we all agree, I would actually
> > deprecate the old one, as that is the one with the problematic name.
> >
> > For the second comment, you are right for the dynamic properties and I
> > will remove them from the document. For the rest (i.e. the "shutdown
> > after exit" and the "attached"), I do not think we can remove them as
> > the user should be allowed to specify them in the configuration.
> >
> > I am hoping to have more opinions on this, and I will open a voting
> > thread as soon as we reach a consensus.
> > Cheers,
> > Kostas
> >
> > On Tue, Oct 22, 2019 at 10:49 AM Dawid Wysakowicz
> > <[hidden email]> wrote:
> >>
> >> Hi,
> >>
> >> I really like the idea of unifying all the ways to set a configuration
> >> option and its keys. I think it nicely aligns with FLIP-59.
> >>
> >> I have two comments though.
> >>
> >> 1. I wonder what are you thoughts on reusing the "parallelism.default"
> >> (CoreOptions.DEFAULT_PARALLELISM) instead of introducing a new key
> >> "execution.parallelism". I am not sure if we should introduce another
> >> key as it might be confusing for users to have two separate config
> >> options for client and cluster side.
> >>
> >> Moreover if we go with a new config option for the client side with a
> >> default value of 1, it makes the cluster side option unusable. Right now
> >> the logic is that if we set the parallelism to -1(the default value) on
> >> the client side, the cluster side configuration is used.
> >>
> >> Would be interested in an opinion of others on that matter. I think we
> >> have few other cases of options that we set on the client side with a
> >> fallback on the cluster side. I think we should have a common approach
> >> to such scenarios.
> >>
> >> 2. If I understood it correctly the "dynamic-properties" (maybe also the
> >> "attached" and "shutdown-on-exit") are an internal options that should
> >> never be set manually by the user through the config file. I think they
> >> are internal and should not be exposed. I wonder if we can make them
> >> less visible somehow. Also I think we do not need to agree on them
> >> through the FLIP process.
> >>
> >> Best,
> >>
> >> Dawid
> >>
> >> On 22/10/2019 10:05, Aljoscha Krettek wrote:
> >>> I think this is good to go to a VOTE. We should, however, make it more prominent that we don’t like the “attached”, “shutdown-on-attached-exit”, and “dynamic-properties” and that we only add them for backwards compatibility.
> >>>
> >>> Best,
> >>> Aljoscha
> >>>
> >>>> On 21. Oct 2019, at 17:48, Kostas Kloudas <[hidden email]> wrote:
> >>>>
> >>>> Hi all,
> >>>>
> >>>> As part of FLIP-73 (the Executors effort), we would like to introduce
> >>>> some new configuration options. These are needed in order to be able
> >>>> to map all the options that the user can specify either
> >>>> programmatically or through the CLI into configuration options.
> >>>>
> >>>> The bylaws specify that every user-facing change should pass through a
> >>>> FLIP, so the FLIP with more details on the new options can be found
> >>>> here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=133631524
> >>>>
> >>>> Please keep the discussion in the mailing list.
> >>>>
> >>>> Thanks,
> >>>> Kostas
> >>
>