On 04 Jun 13:06, Albert Cervera i Areny wrote: > 2015-06-03 12:40 GMT+02:00 Cédric Krier <[email protected]>: > > On 03 Jun 12:12, Albert Cervera i Areny wrote: > >> 2015-06-03 11:21 GMT+02:00 Cédric Krier <[email protected]>: > >> > On 02 Jun 11:46, Cédric Krier wrote: > >> >> On 02 Jun 11:06, Albert Cervera i Areny wrote: > >> >> > 2015-05-29 15:39 GMT+02:00 Cédric Krier <[email protected]>: > >> >> > > On 29 May 14:40, Albert Cervera i Areny wrote: > >> >> > >> Also note that tryton locks the whole table even when two users are > >> >> > >> not competing for the same product. > >> >> > >> > >> >> > >> I created a review [1] some time ago to fix that although it > >> >> > >> eventually was commited something much simpler, you can dig in the > >> >> > >> reviews, where per product advisory locks were used. > >> >> > > > >> >> > > Because it doesn't work. It will require a lot of extrat code to be > >> >> > > managed correctly: lock at creation, on both side of the write, on > >> >> > > delete etc. > >> >> > > > >> >> > > >> >> > I've been revisiting this issue, I don't understand why a lock is > >> >> > necessary outside assign_try. > >> >> > > >> >> > Let me put an scenario: > >> >> > > >> >> > There's 1 unit of product A on warehouse X. > >> >> > > >> >> > There are two concurrent transactions: > >> >> > > >> >> > a) does assign_try to try to ship the product to a customer > >> >> > b) moves the product from warehouse X to warehouse Z without > >> >> > assign_try (that may include a write, create, or delete, doesn't > >> >> > really matter for the case) > >> >> > > >> >> > So let's see what happens with and without those locks. > >> >> > > >> >> > If we just have the lock on assign_try there can be two possibilities: > >> >> > > >> >> > 1.- a) is executed *after* b) so as the product has been moved from X > >> >> > to Z and there are 0 units of product A in X, assign_try "fails" and > >> >> > stock can not be picked > >> >> > 2.- a) is executed *before* b) so we deliver the goods to the customer > >> >> > *and* also make the move from X to Z which means that the stock of > >> >> > product A is no -1 on warehouse X > >> >> > > >> >> > We probably don't want the second scenario so we would add the > >> >> > suggested lock on write/create/delete. > >> >> > > >> >> > BUT what happens if those transactions are NOT concurrent and we DO > >> >> > have the lock? > >> >> > > >> >> > Take the second possibility where a) is executed *before* b). What > >> >> > happens is that we end up having -1 units of product A on warehouse X > >> >> > anyway. > >> >> > > >> >> > What I see is that as long as you want to ensure that the reservation > >> >> > process works as expected *all* stock moves must go through > >> >> > *assign_try* because it is this workflow that ensures the expected > >> >> > behaviour, not the lock mechanism. > >> >> > > >> >> > So I don't see any reason why we should add the lock anywhere else > >> >> > apart from assign_try. > >> >> > > >> >> > What am I missing? > >> >> > >> >> Maybe but there are no proof that finer grain locking system is better > >> >> or faster than a big one. For sure, finer grain is more complex and > >> >> could have dead locks. > >> > > >> > Also your analysis takes in consideration only the Tryton's modules. But > >> > we could imagine a valid case where you don't use assignation to make > >> > move. Like I tried to explain before you could build different workflow > >> > for picking which could write directly "done" move and there is no > >> > reason to not allow both working at the same time. > >> > >> But in that case the alternative workflow would have to take care of > >> locking anyway. So that doesn't make one option better than the other. > >> > >> With current implementation it would have to do: > >> > >> Transaction().cursor.lock(cls._table) > >> > >> but if an alternative locking mechanism we could provide a function to > >> be used by those alternatives to use: > >> > >> StockMove.lock(products) > > > > I don't agree. There are no reason such code that create a "done" move > > to call lock. It doesn't need to prevent anything from happening. It > > will be a design mistake. > > Indeed we already have such code in the stock module, it is the > > inventory. > > > > I see your point. > > >> Even more, it could be considered to add such API even without > >> changing the default locking mecanism. > >> > >> As you said, probably Raimon's scenario may not benefit from my > >> proposed locking mechanism but as there are several needs and > >> scenarios, maybe it would be interesting to have a common API to try > >> to improve compatibility between several alternatives. > >> > >> Also, maybe it would improve Raimon's situation if instead of raising > >> a backtrace a proper message such as "Currently other users are making > >> lots of stock moves. Please, try again later." was shown. That > >> probably would need to be handled at "trytond" and "stock" module > >> level, by providing a new kind of exception which did the retries and > >> only after those retries it was risen. > > > > I don't agree we will be just hidding the problem. Also why asking the > > user to try later when trytond can do it with the retry configuration. > > Also the problem is not the exception (it is just the revealing), the > > problem is the long transaction. > > > > There is always the possibility that the given retry configuration is > not enough in an exceptional case. > > So for me it is better to send an error message that can be understood > instead of a backtrace. > > The exact message could be completely different, I just meant to avoid > the backtrace.
I think this is wrong. This is not how we will make a better software. The proposed way to hide problem has the same effect than silent error, nobody cares to fix them. -- Cédric Krier - B2CK SPRL Email/Jabber: [email protected] Tel: +32 472 54 46 59 Website: http://www.b2ck.com/
