Feature/concurrent category creation

[reftel]

Adding multiple entries with the same new criterion fails for us with the Hibernate adapter, since each transaction sees that the criterion is missing and tries to create it, after which all but one of the transactions fail due to the criterion now being present. Retrying in case of error solves this, as each retry should have created at least one of the categories, ensuring that we are making progress. The number of retries is limited to the number of criteria to ensure that we eventually do fail if the root cause is something else. An earlier version of this patch only performed retries when encountering a failure due to a constraint violation. This has been changed to perform retries on any kind of failures, as the concurrent writes can manifest themselves are other types of errors as well. Specifically, we got lock timeout errors in other transactions when the transaction that wrote first stalled before completing the commit.

[cloudfeeds-jenkins-ci]

Can someone in the Cloud Feeds team review this patch?

[usnavi]

Thanks for the PR! The committers employed by Rackspace now also support some other projects, so we won't be able to look at this immediately. We'll take a look at this in the next week or two.

[usnavi]

ok to test.

[usnavi]

@magnusr - Thanks for the PR! We don't use the hibernate adapter anymore, so I'm not that familiar with the code. I have a couple of questions about this, though. I'm not familiar with hibernate either, so please excuse my ignorance. @shintasmith might have some ideas.

1) What sort of error is thrown if the category already exists? If this can be detected, couldn't the exception be caught earlier and the code continue to iterate through the the list of categories without retries? 2) Could the exception be caught within the for loop rather than outside it? This would also prevent adding the same categories over and over again during the retries.

Overall, I would think a solution could exist that would only have to retry categories which encountered an error (and hadn't been recognized to already be existing in the table).

[reftel]

Hi,

thanks for reviewing this!

The exact error thrown, and when it is thrown, depends on the database. The error will typically not include information about which of the categories already existed.

We use Oracle in production, and it throws an exception when committing, meaning after the SimpleSessionAction/ComplexSessionAction has returned. Therefore, the catch needs to be around the call to performSimpleAction/performComplexAction.

For some of our testing, we use H2, and there we haven gotten errors due to lock timeouts when inserting when we stall a thread that has already inserted something in the category table but not yet committed. This is why the code was changed to no longer look at the specific exception thrown, but retry on any exception.

The code already tries to detect that categories already exist, but it's not possible to detect that a separate transaction has interred new ones but not committed them. That's the situation that this change attempts to solve.

If we really, really, want to retry only the categories that encountered errors, then they could be inserted and committed in separate transactions. That would, however, mean an overhead in the non-contending cases (which are by far more common) due to the increased number of transactions, and would also make it hard to roll the changes back if something else fails.

[usnavi]

Thanks for the info. Would you mind adding a comment to the code describing the motivation & the reasoning for the particular change. This sounds fine by me, but future devs might have concerns if they don't have the background information.

lgtm - @shintasmith please take a look and merge if you are cool with this.

thanks again @magnusr!

[reftel]

Comment added.

[shintasmith]

lgtm 👍 Thank you @magnusr