On 08/09/2016 11:39 AM, Amos Jeffries wrote: > On 10/08/2016 4:41 a.m., Alex Rousskov wrote: >> On 08/09/2016 05:38 AM, Eduard Bagdasaryan wrote: >>> On 08/08/2016 02:17 PM, Amos Jeffries wrote: >>>> * helper::SubmissionFailure is also changing what was previously >>> Helper::Unknown result codes to Helper::Error. >>> - Helper::Error is one of the helper output codes, it means success. >>> Obviousy when hlp == NULL, there is no success going on. >>> - please revert to sending helper::Unknown code. >> >>> Reverted. Note that for helper::Unknown some callers print a bit >>> misleading error message, when requests are dropped, e.g.: >>> "ERROR: storeID helper returned invalid result code. Wrong helper?" >> >> and the documentation now lies: >> >>> + Two [on-persistent-overload] actions are supported: >>> + >>> + die Squid worker quits. This is the default behavior. >>> + >>> + err Squid treats the helper request as if it was >>> + immediately submitted, and the helper >>> immediately >>> + responded with an error. This action has no >>> effect on >>> + the already queued and in-progress helper >>> requests. >> >> Perhaps SubmissionFailure() should use Helper::Unknown when its hlp >> parameter is nil and Helper::Error in other cases? >> >> >> FYI: The original intent and the documentation say that we should return >> Helper::Error. Yes, Helper::Error means a "successfully parsed" helper >> response [with helper-specific semantics], and IIRC, that is what we >> wanted to relay to the helper-using code because that code has a better >> chance of doing the right thing when receiving Helper::Error (compared >> to handling Helper::Unknown). >> >> We also considered adding a new result code for overload but rejected >> that idea for several reasons, including the expectation that the >> helper-using code already has logic to handle Helper::Error and that >> logic will work for on-persistent-overload=err cases well. >> >> Please note that I am not advocating a _change_ in the current Squid >> default behavior. I am only questioning whether it is a good idea to >> return Helper::Unknown instead of Helper::Error with >> on-persistent-overload=err. Amos' arguments do not apply to that >> specific case AFAICT because there is no "change" in that specific case >> (it is a new behavior). >> > > At the moment Helper::Unknown means a problem in Squid side of things > (fail). BH means a problem internally to the helper (fail). And both > OK/ERR mean helper decision was made (success). > The naming may not great, and the way they map down to boolean > allow/deny type values is very painful, but thats what we have to work > with currently.
Yep, that matches both my understanding and motivation to return ERR in the explicitly configured on-persistent-overload=err case. > The decision of whether Unknown maps to ERR is something each different > helper interface needs to decide for itself based on what its purpose is. The decision what fake response object to give the helper code [when helper itself is overloaded] should be up to the admin. The admin wants "as if the helper returned ERR". We can (and perhaps should) add more on-persistent-overload values later, of course. Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
