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

Wed Sep 3 13:50:04 CEST 2014

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

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
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20140903/052681e8/attachment.htm>