[Cubicweb] NativeSQLSource.doexec auto-rollback

aurélien campéas aurelien.campeas at gmail.com
Sun Dec 28 00:39:05 CET 2014


2014-12-27 18:36 GMT+01:00 Christophe de Vienne <christophe at unlish.com>:

> Hi everyone,
>
>
Hi Christophe,


> We are running into a behavior that feels wrong to me, while using the
> worker cube.
>
> The worker cube uses the task workflow to make sure only one worker gets
> to execute it. Hence, to acquire a task before executing it, it simply
> attempt to change its state.
>
> Doing so, it actually write a new entry in the database: a relation
> between the task and the 'executing' state. If the task is already being
> executed, ie if its state it already 'executing', the update fails with
> a duplicate key violation, and the worker simply pass to the next task.
>
> So far so good.
>
> The problem is that when the duplicate key violation occur, which is
> expected to happen sometimes with several workers, here is what we get
> in the logs:
>
> cubicweb.sources.system 2014-12-27 16:57:07,780 CRITICAL sql many:
> 'INSERT INTO in_state_relation ( eid_from, eid_to ) VALUES (
> %(eid_from)s, %(eid_to)s )'
>  args: [{'eid_to': 2240, 'eid_from': 1250592}]
> dbms message: 'duplicate key value violates unique constraint
> "in_state_relation_p_key"\nDETAIL:  Key (eid_from, eid_to)=(1250592,
> 2240) already exists.\n'
> cubicweb.sources.system 2014-12-27 16:57:10,072 CRITICAL transaction has
> been rolled back
>
>
> I consider this a problem because it is in no way a critical error: it
> is expected to happen sometimes, and if it does it is perfectly fine. It
> should not even be a warning.
>
>
> Digging a little in the source and looking for the 'transaction has been
> rolled back', I found the NativeSQLSource.doexec function, which is
> basically:
>
>     def doexec(self, cnx, query, args=None, rollback=True):
>         cursor = cnx.blablabla
>         try:
>             cursor.execute()
>         except Exception as ex:
>             self.critical('sql: xxx' % ex.xxx)
>             if rollback:
>                 cnx.rollback()
>                 self.critical('transaction has been rolled back')
>             # some clever code to translate the exception in a CW one
>             # if possible
>
> Several things are bothering me:
>
> - The 'rollback' feature, if anyhow useful (I do not think it is),
>   should not have such a default: doexec is, in my understanding, a low
>   level function. And rolling back a transaction is a high-level
>   decision. But since the doexec function is called by many intermediate
>   functions without passing any 'rollback' parameter, we end up with a
>   high-level decision made in a low-level function.
>

At this point in the flow, the low-level transaction has been rolled back,
and we cannot defer for too long emitting a rollback event at the cw "cnx"
level.


>
> - If an error occur at the sql level, it will be transmitted or
>   translated to a CW error, which is fine. But if the exception is
>   raised and handled in a higher level function, why do we make it a
>   critical error?
>
> - Admitting that rolling-back automatically on errors is wanted in some
>   cases, how can it be considered 'critical'? The caller will receive
>   the original exception anyway, it is its job to decide if it is
>   critical or not.
>


> So basically, I would like to propose 2 things:
>
> - Remove the 'critical' logs, and change them into into 'debug', or
>   'info'.
>

+1


>
> - Remove the 'rollback' option, or at least change its default value.
>   I really thing this feature has nothing to do in this place.
>

Should this be moved up to the Connection object ?

<https://lists.cubicweb.org/mailman/listinfo/cubicweb>Regards,
Aurélien.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cubicweb.org/pipermail/cubicweb/attachments/20141228/595fd060/attachment-0165.html>


More information about the Cubicweb mailing list