[sakai-core-team] from Jun 26: KNL-815

[Charles Hedrick]

I said I would review KNL-815.

First, this problem was triggered at Rutgers primarily by Lessons. I fixed Lessons so that it won’t trigger the problem, even without a kernel fix.I do this by passing a very large maxsize to addNewUser. The existing code does the necessary row lock in that case.

However Rutgers is running with diff.1 from KNL-815, and has been since 2011.

But note that there is a potential for trouble with this change if you don’t also fix KNL-433. I make sure that transactions always close or rollback, by adding a commit to SakaiPoolableConnectionFactory:passivateObject.

I honestly have no idea whether these changes are still needed. I think there’s a pretty good chance that this was actually a mysql bug. The deadlock didn’t involve a sequence that you’d expect would need to be locked. It involved a single update statement. So it’s not the case that I can prove from inspection that there’s actually a bug. We eventually found a mysql bug that would appear to have caused the problem. I couldn’t back out of my fix, because we couldn’t do mysql updates during the semester. 

But my inclination would be not to do anything about KNL-815 unless others see the symptom.

I’m more concerned with KNL-433. As I recall there was a fairly visible symptom associated with KNL-433, namely KNL-797. People made changes to the site in Site Info, and they didn’t always show up for hours, because the commit didn’t happen. KNL-799 would have resolved it, but that hasn’t been resolved either. 

So my summary is: If KNL-797 is still happening, then I think you should add the commit to passivateObject or possibly one of the proposed fixes for KNL-433/KNL-799. I wouldn’t do the patch in KNL-815, as i think that was ultimately a Mysql bug.

As to KNL-433 / 799, the question was whether it is possible for transact to not do either commit or rollback. It appears that this depends upon whether any error occurs that wasn’t trapped. My recommendation was to do a rollback in the finally if there hadn’t been a commit or rollback. This is a pretty classic use of finally. The other suggestion was to move the RunTimeException test to the end, after the SqlException, and make it Exception rather than RunTimeException. I’d much rather see the code simplified so that the finally has if (connection != null && ! commit done) rollback. With that, the RunTimeException test isn’t needed at all, and the rollback can be removed from the other tests.