Github user rafaelweingartner commented on the issue:

    https://github.com/apache/cloudstack/pull/1935
  
    @nvazquez great work.
    However, there is a catch there that I think you might have overlooked. 
This problem is caused by the method extraction I suggested.
    
    If you take a look at the code before the extraction, every time that an 
exception is thrown, the code was setting the variable `rollBackState = true`. 
This happens at lines 287, 305, and 313. Now that the code was extracted, 
setting those variables to `true` does not work anymore, because of the context 
those variables are declared change.
    
    In my opinion, this code was kind of weird before. It was throwing an 
exception that is caught right away and setting a control variable to be 
executed on `finally` block. The only reason I see for this is that if other 
exceptions that are not the ones generated at lines 292, 310, and 325 happen, 
and we do not want to execute the rollback for them. However, this seems error 
prone, leading to database inconsistencies.
    
    I would change the "rollback" code (lines 342-345) to the catch block.
    
    I do not know if I have been clear, we can discuss this further. I may have 
overlooked some bits of it as well (it is a quite complicated bit of code).



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to