[erlang-patches] allow use of proplists in supervisor start specs

Wed Sep 3 15:43:27 CEST 2014

On Wed, Sep 3, 2014 at 1:50 PM, Siri Hansen <erlangsiri@REDACTED> wrote:

> Hello!
>
> It looks like we have reached consensus regarding the default values :)
> Thanks again for all input!
>
> Over the summer we have had some internal reviews of the documentation and
> I got some feedback on the naming. Specifically, questions are raised about
> the keys 'name', 'intensity' and 'period'. With the old tuple format, these
> three values are named Id, MaxR and MaxT respectively, and the questions
> are if they should be changed at all and if so, are the suggested names
> "the best"...
>
> Personally, I think that changing Id to 'name' might not be necessary.
> Partly because this value is discussed as the "child identifier" everywhere
> in the documentation and partly because the type definition of it is term()
> (and not atom() or string()). So my suggestion is to change this key to
> 'id'.
>
> The reasoning about 'intensity'/MaxR and 'period'/MaxT is more complex, I
> think. I agree that the old variable names (at least MaxT), are not super
> good. One reason for keeping to something similar to this is, however, that
> these names are used when describing the mechanism of restart escalation in
> some of the erlang books that already exist. And if you are familiar with
> the old format, then switching to the new format will of course be easier
> if the key names are easy to map. But on the other hand, if we could find
> really good names to use instead, then maybe it would be worth the change
> in order to ease the introduction for newcomers. Again personally, I am not
> sure that 'intensity' and 'period' are so much better than the old names...
>
> What we are talking about here is some kind of "restart frequency",
> although it is not really a frequency either - at least not a constant
> frequency. It's the limit for the number of restarts within a time
> window... Are the names max_restarts or restart_limit better than
> intensity. Is period ok, or time_frame, time_window ... ? Or should we
> stick to max_time to keep close to the old names?
>
> Would it make sense to put both values into a tuple using only one key
> (restart_frequency, ...?) in the map?
>
> So many questions... naming is hard! Any thoughts?
>
> /siri
>

Would it make sense to have a single property "max_restart_frequency" whose
value is a tuple {R, T} ?

>
> 2014-07-04 12:13 GMT+02:00 Siri Hansen <erlangsiri@REDACTED>:
>
> I have pushed the current status of this job to github. Comments still
>> welcome!
>> /siri
>>
>>
>> https://github.com/sirihansen/otp/compare/erlang:980d7fa32128d43d36fb9527df1967eff801be9c...siri/sup-spec-maps/OTP-11043
>>
>>
>> 2014-07-03 20:24 GMT+02:00 DeadZen <deadzen@REDACTED>:
>>
>> +1 on the latest revision
>>>
>>> On Thu, Jul 3, 2014 at 9:57 AM, Siri Hansen <erlangsiri@REDACTED>
>>> wrote:
>>> > Hello,
>>> >
>>> > thanks for the feedback! We have decided to take your comments into
>>> account
>>> > and change the default values as follows:
>>> >
>>> > Intensity/Perior: 1/5
>>> > Shutdown for worker: 5000
>>> > Shutdown for supervisor: infinity
>>> >
>>> > The reasoning behind it is that
>>> >
>>> > 1. as a few of you commented, it is probably not a good idea to have a
>>> > default without restarts, as restarts is a very important property of
>>> the
>>> > supervisor tree. Using 1 restart in 5 seconds was a compromise between
>>> my
>>> > first intention of "being extreme" and the suggestions from the
>>> list... And
>>> > also - if a process can not be restarted on the first attempt, then
>>> maybe it
>>> > is not so great a chance that it will succeed on the second attempt
>>> either.
>>> > And 5 seconds is a very good number!!
>>> >
>>> > 2. to avoid the confusion of why the terminate function is not
>>> executed, we
>>> > decided not to use 'brutal_kill'. Which number to use is probably not
>>> that
>>> > important, so we chose 5000 - which we all know is proven to be "the
>>> best
>>> > timeout value" ;)
>>> >
>>> > 3. the recommended shutdown value for supervisors is 'infinity', and I
>>> think
>>> > that should be reflected in the default. Having the same (integer)
>>> value as
>>> > for workers can give very strange results.
>>> >
>>> > Further comments are still welcome... :)
>>> >
>>> > /siri
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > 2014-07-01 14:43 GMT+02:00 Daniel Luna <daniel@REDACTED>:
>>> >
>>> >> I have to mention that I think brutal_kill is a really bad choice for
>>> a
>>> >> default.
>>> >>
>>> >> It breaks the expectation in Erlang that supervisors can never crash.
>>> An
>>> >> expectation that is in the core of what makes Erlang the great
>>> language that
>>> >> it is.
>>> >>
>>> >> Dangling process trees are not fun to deal with (and don't happen on
>>> a VM
>>> >> crash).
>>> >>
>>> >> To preserve the correctness and trust of supervision trees the default
>>> >> should be either infinity or depend on the child type.
>>> >>
>>> >> For worker children brutal_kill is fine. For supervisor children
>>> infinity
>>> >> is the only sensible choice.
>>> >>
>>> >> On Jul 1, 2014 7:06 AM, "Loïc Hoguin" <essen@REDACTED> wrote:
>>> >>>
>>> >>> Just wanted to say I agree with everything José said, except 3. the
>>> >>> different defaults for different process types. It makes things too
>>> hard to
>>> >>> keep track of.
>>> >>>
>>> >>> I think brutal_kill is a great choice because it makes it easier for
>>> the
>>> >>> user to stumble upon cold-start issues when restarting their
>>> >>> application/release. The VM may crash so there's never a guarantee
>>> to run
>>> >>> shutdown functions anyway, and too few people forget to test cases
>>> when
>>> >>> these didn't run, leading to potential production disasters.
>>> >>>
>>> >>> On 06/30/2014 04:37 PM, José Valim wrote:
>>> >>>>
>>> >>>> I have some feedback based on my experience in writing and teaching
>>> >>>> Erlang/Elixir.
>>> >>>>
>>> >>>> 1. I would always require the supervision strategy. When I am
>>> >>>> prototyping, it is usually hard for me to reason about the
>>> intensity and
>>> >>>> restarts, but I can always reason about the supervision strategy.
>>> All
>>> >>>> teaching materials I know of also discuss the supervision right
>>> away, so
>>> >>>> having it explicit shouldn't hurt those cases.
>>> >>>>
>>> >>>> For such cases, maybe we should use the format of {{strategy(),
>>> map()},
>>> >>>> [children()]} (which is similar to what we have today).
>>> >>>>
>>> >>>> 2. I also agree with Richard. Having a default of 0 restarts may
>>> cause
>>> >>>> confusion and forcing people to define them won't help when they
>>> still
>>> >>>> have no idea of what values they should use (because it is
>>> prototyping /
>>> >>>> early deploys). I usually fine tune those values based on the
>>> production
>>> >>>> system reports so having saner defaults, like Ulf's, may make more
>>> >>>> sense.
>>> >>>>
>>> >>>> 3. The child_spec defaults are great and similar to the ones we use
>>> in
>>> >>>> Elixir with the only difference being the shutdown value. We have a
>>> >>>> default of 5000, which is the one usually recommended, but we set
>>> it to
>>> >>>> "infinity" if the type is a "supervisor" (which is the OTP docs
>>> >>>> recommendation if I remember correctly).
>>> >>>>
>>> >>>> Regarding 3., if the recommendation and the consensus in actual
>>> systems
>>> >>>> is to use brutal_kill for shutdown, I will gladly change Elixir to
>>> make
>>> >>>> it consistent with OTP defaults.
>>> >>>>
>>> >>>> Thanks for opening the changes for discussion!
>>> >>>>
>>> >>>> *José Valim*
>>> >>>> www.plataformatec.com.br <http://www.plataformatec.com.br/>
>>> >>>> Skype: jv.ptec
>>> >>>> Founder and Lead Developer
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> _______________________________________________
>>> >>>> erlang-patches mailing list
>>> >>>> erlang-patches@REDACTED
>>> >>>> http://erlang.org/mailman/listinfo/erlang-patches
>>> >>>>
>>> >>>
>>> >>> --
>>> >>> Loïc Hoguin
>>> >>> http://ninenines.eu
>>> >>> _______________________________________________
>>> >>> erlang-patches mailing list
>>> >>> erlang-patches@REDACTED
>>> >>> http://erlang.org/mailman/listinfo/erlang-patches
>>> >
>>> >
>>> >
>>> > _______________________________________________
>>> > erlang-patches mailing list
>>> > erlang-patches@REDACTED
>>> > http://erlang.org/mailman/listinfo/erlang-patches
>>> >
>>>
>>
>>
>
> _______________________________________________
> erlang-patches mailing list
> erlang-patches@REDACTED
> http://erlang.org/mailman/listinfo/erlang-patches
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20140903/b3373819/attachment.htm>