Fix compareAndSwap() in SQLMetadataConnector

[jon-wei]

This PR fixes SQLMetadataConnector.compareAndSwap() by adding " FOR UPDATE" to the initial read and using REPEATABLE_READ isolation level for the transaction.

This method is used by druid-basic-security extension APIs, and the broken CAS method was causing issues where some concurrent update requests (such as assigning roles to a user) failed to take effect.

A test is added that concurrently makes 100 role assignment requests and later concurrently removes the 100 roles.

[gianm]

Nice catch!!

Do we need similar changes in IndexerSQLMetadataStorageCoordinator's announceHistoricalSegments method? There's a similar kind of CAS going on there.

[jon-wei]

Marking this WIP, while testing just found that Postgres has somewhat different behavior from Derby/MySQL:

https://www.postgresql.org/docs/10/transaction-iso.html#XACT-REPEATABLE-READ

...if the first updater commits (and actually updated or deleted the row, not just locked it) then the repeatable read transaction will be rolled back with the message ERROR: could not serialize access due to concurrent update because a repeatable read transaction cannot modify or lock rows changed by other transactions after the repeatable read transaction began. When an application receives this error message, it should abort the current transaction and retry the whole transaction from the beginning.

So I'll need to add retries for that case for postgres

Do we need similar changes in IndexerSQLMetadataStorageCoordinator's announceHistoricalSegments method? There's a similar kind of CAS going on there.

I'll take a look at that, thanks!

wip

[jon-wei]

Added handling for Postgres serialization_failure error, removed WIP tag

[clintropolis]

lgtm 👍

[clintropolis]

nit: method argument doesn't seem like it needs to be on a new line... alternatively any reason not to just inline this whole method in the compareAndSwap method?

[jon-wei]

With the somewhat non-trivial boolean conditions I felt like it looked nicer as a separate method

[surekhasaharan]

LGTM

[surekhasaharan]

Do we need similar changes in IndexerSQLMetadataStorageCoordinator's announceHistoricalSegments method? There's a similar kind of CAS going on there.

I'll take a look at that, thanks!

Does that code need any fixes ?

[jon-wei]

Do we need similar changes in IndexerSQLMetadataStorageCoordinator's announceHistoricalSegments method? There's a similar kind of CAS going on there.

I'll take a look at that, thanks!

Does that code need any fixes ?

I'm still looking into that, but it can be a separate PR if there are similar issues there.