On 09/06/2016 05:11 PM, Justin Stephenson wrote:
On 09/06/2016 10:57 AM, Petr Cech wrote:
On 09/06/2016 04:17 PM, Justin Stephenson wrote:


On 09/05/2016 10:20 AM, Petr Cech wrote:
On 09/05/2016 04:05 PM, Fabiano Fidêncio wrote:
Petr,

On Mon, Sep 5, 2016 at 3:43 PM, Petr Cech <pc...@redhat.com> wrote:
On 09/05/2016 09:57 AM, Fabiano Fidêncio wrote:

Petr,

I see you have updated the OPT_MAX_CHILDREN_DEFAULT to 10 instead of
50. However, you haven't update the value on sssd.conf.5.xml:

diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index
ae291e0fc8f2f9afabcdf32f18a5ec12252bbbbf..1bf3e799047d9c722487be8657bbee5cfd479cdd




100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -2464,6 +2464,18 @@ subdomain_inherit = ldap_purge_cache_timeout
                     </listitem>
                 </varlistentry>

+                <varlistentry>
+                    <term>proxy_max_children (integer)</term>
+                    <listitem>
+                        <para>
+                            The number of preforked proxy children.
+                        </para>
+                        <para>
+                            Default: 50
                                   here: ^^^

+                        </para>
+                    </listitem>
+                </varlistentry>
+
             </variablelist>
         </para>

Apart from this minor the patch seems to be following everything
that
was requested during the review process. However, I'm not
comfortable
with the text used to describe the new option, so adding there a bit
more information would be super. Like, I don't know what's the
influence of the preforked proxy children to the rest of the code
(probably because I'm a newbie here ;-)), but would be nice to
have it
clear in the documentation (for newbies like myself ;-)).

Best Regards,
--
Fabiano Fidêncio


Hi Fabiano,

thanks for code review. I fixed the default value in man page and I
reformulated description. Is it better?

Regards

--
Petr^4 Čech

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org






Looking at the changes in the manual ...

+                            In busy environments it is possible sssd
runs out of
+                            available child slots and starts queuing
requests
+                            in proxy mode. This option introduces the
number of
+                            preforked proxy children.

I personally go for something like:

"This option introduces the number of pre-forked proxy children. It's
useful for busy environments* where sssd may run out of available
child slots, which would cause some issues due to the requests being
queued".

*: Not sure whether busy environments is something clear for everyone
...

IMO the patch is good to go as soon as we have this part done/reviewed
by a native speaker.
Maybe Justin can help us here?

Best Regards,
--
Fabiano Fidêncio

Fabiano,

I took your suggestion, thanks. I don't know right term for 'busy
environments'. I will be glad if native speaker help me with the right
formulation of description.

Something like "it is useful for high-load SSSD environments" or "it is
useful for larger environments" may work - whichever you prefer.

Kind regards,
Justin Stephenson

Hello,

new version attached. Description is:

This option specifies the number of pre-forked
proxy children. It is useful for high-load SSSD
environments where sssd may run out of available
child slots, which would cause some issues due to
the requests being queued.

After hint from Michal I replaced introduces by specifies.

Is this version linguistic right?

This description looks good to me.

-Justin

Thanks, Justin.

--
Petr^4 Čech
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Reply via email to