<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2014-12-29 12:40 GMT+01:00 Christophe de Vienne <span dir="ltr"><<a href="mailto:christophe@unlish.com" target="_blank">christophe@unlish.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
Le 28/12/2014 00:39, aurélien campéas a écrit :<br>
<span class="">><br>
><br>
> 2014-12-27 18:36 GMT+01:00 Christophe de Vienne <<a href="mailto:christophe@unlish.com">christophe@unlish.com</a><br>
</span>> <mailto:<a href="mailto:christophe@unlish.com">christophe@unlish.com</a>>>:<br>
<br>
[...]<br>
<span class=""><br>
>     Digging a little in the source and looking for the 'transaction has been<br>
>     rolled back', I found the NativeSQLSource.doexec function, which is<br>
>     basically:<br>
><br>
>         def doexec(self, cnx, query, args=None, rollback=True):<br>
>             cursor = cnx.blablabla<br>
>             try:<br>
>                 cursor.execute()<br>
>             except Exception as ex:<br>
>                 self.critical('sql: xxx' % ex.xxx)<br>
>                 if rollback:<br>
>                     cnx.rollback()<br>
>                     self.critical('transaction has been rolled back')<br>
>                 # some clever code to translate the exception in a CW one<br>
>                 # if possible<br>
><br>
>     Several things are bothering me:<br>
><br>
>     - The 'rollback' feature, if anyhow useful (I do not think it is),<br>
>       should not have such a default: doexec is, in my understanding, a low<br>
>       level function. And rolling back a transaction is a high-level<br>
>       decision. But since the doexec function is called by many intermediate<br>
>       functions without passing any 'rollback' parameter, we end up with a<br>
>       high-level decision made in a low-level function.<br>
><br>
><br>
> At this point in the flow, the low-level transaction has been rolled back,<br>
> and we cannot defer for too long emitting a rollback event at the cw<br>
> "cnx" level.<br>
<br>
</span>What do you mean by "the low-level transaction has been rolled back"?<br>
Do you refer to something lower-level than doexec?<br>
Why would an error rollback the transaction?<br></blockquote><div><br></div><div>After reception of an IntegrityError, you can be sure that the sql backend</div><div>wiped the current transaction clean.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
><br>
>     - If an error occur at the sql level, it will be transmitted or<br>
>       translated to a CW error, which is fine. But if the exception is<br>
>       raised and handled in a higher level function, why do we make it a<br>
>       critical error?<br>
><br>
>     - Admitting that rolling-back automatically on errors is wanted in some<br>
>       cases, how can it be considered 'critical'? The caller will receive<br>
>       the original exception anyway, it is its job to decide if it is<br>
>       critical or not.<br>
><br>
><br>
><br>
>     So basically, I would like to propose 2 things:<br>
><br>
>     - Remove the 'critical' logs, and change them into into 'debug', or<br>
>       'info'.<br>
><br>
><br>
> +1<br>
<br>
</span>Ticket+patch: <a href="http://www.cubicweb.org/ticket/4801120" target="_blank">http://www.cubicweb.org/ticket/4801120</a><br>
<br>
I chose 'debug' and 'info' levels for the rollback and sql error logs.<br>
I wonder if using 'warning' could make sense.<br></blockquote><div><br></div><div>Choosing warning would keep the information more visible.</div><div>Integrity errors are rare events in general and we may want</div><div>to keep them easily spottable in a log stream.</div><div> </div><div>Regards,</div><div>Aurélien.</div><div><br></div></div></div></div>