Consistency issues with different connections (eg read/write replicas) #289
Comments
|
Indeed this seems tricky to handle for this bundle 😕 The Possibly one could decorate Or we find a way of ensuring the same connection hash is used for primary and the read replica inside For now I would also suggest to configure a connection without replicas for the test environment then. |
|
@Tristan971 @prohalexey could you maybe test https://github.com/dmaicher/doctrine-test-bundle/compare/connection_key?expand=1 ? Its some quick proof of concept to allow forcing usage of a specific key for keeping the connection in the static array. Not sure yet how to expose that (via config maybe) but for this PoC I quickly enabled defining parameters for that. This also allows to define/overwrite the key that is used per replica. So for testing this PoC with a config like this doctrine:
dbal:
default_connection: default
connections:
default:
url: '%env(resolve:DATABASE_URL)%'
...
replicas:
ro:
url: '%env(resolve:DATABASE_RO_URL)%'you would need to define Symfony parameters to force using the same key for both parameters:
dama.connection_key.default: some_custom_key
dama.connection_key.default.ro: some_custom_keyWDYT? Would this solve your problem? |
|
@dmaicher I think yes, if i could set the same keys for all connections. BTW json_encode does not garauntee the order of elements in the array, so even if connections have the same connection params, they may produce different sha1 hashes(due to sorting). By default, you have to ksort connection params before json_encode. |
Do you have a source for that? I tested it and at least on PHP 8.3 this always generates exactly the same json. |
The first part is https://www.rfc-editor.org/rfc/rfc7159.html#section-1
The second part is that keys in php array could be in the different order, but with the same values(values loaded from configuration), so it will produce the different string. |
Ok but that is completely unrelated to the php implementation of
How should this happen though? Also certain here that the order of keys will not change really. Its build deterministically from the Bundle configuration. Anyway this is off-topic. I don't see an issue with the implementation. If you encounter any problems with changing hashes then please open a new issue. |
|
@Tristan971 @prohalexey do you think it would make sense to always (by default) use the same underlying driver connection for the primary and the read replicas? I can't really think of a scenario where you would need different connections and this would result in having those read inconsistencies due to non-committed transactions... 🤔 |
|
Well, there's a reasonable argument that if someone is testing that something uses the primary/secondary specifically, then this side-effect/"bug" was actually a feature. But in my opinion that's not very likely, so as long as it's documented in release notes then someone writing tests that are this specific will figure it out. |
|
I just tested this solution, with connection keys. Looks good for me as well. |
|
@zilionis what do you think about #289 (comment)? Any opinion on using the same key by default for primary and read replicas? |
|
You can find a WIP PR here that suggests to make this configurable and to use the same key for primary and replicas by default. Maybe you could give it a try. So for a DBAL config like doctrine:
dbal:
default_connection: default
connections:
default:
url: '%env(resolve:DATABASE_URL)%'
...
replicas:
ro:
url: '%env(resolve:DATABASE_RO_URL)%'by default the connection key If needed this could be changed like dama_doctrine_test:
connection_keys:
default: custom_key #similar to default config this would use "custom_key" as key for primary and replicasdama_doctrine_test:
connection_keys:
default:
primary: custom_key_primary
replicas:
ro: custom_key_read_only_replicaDoes this make sense to you? |
|
@dmaicher Hey, I tested PR, for me looks good. |
|
released: https://github.com/dmaicher/doctrine-test-bundle/releases/tag/v8.1.0 Thanks for the input 👍 |
The issue is similar to #284, but easier to catch in a more common setup, like so (Symfony 6.4):
In this case, you might end up with invisible writes if they're stuck within a transaction on the
defaultconnection, but you're doing a read with the readonly version (default.rois its name with this config)Indeed, this yields 2 different static connections, since the hash in
doctrine-test-bundle/src/DAMA/DoctrineTestBundle/Doctrine/DBAL/StaticDriver.php
Line 31 in f10de29
eg:
This isn't surprising, to be fair, and I don't think the project can do much for it, but hopefully this saves someone some time.
The solution being to make sure your tests use a single connection per database (at least within contexts where you expect to see writes done by tests). Unfortunately Symfony doesn't make this easy.
In principle, you could add
config/test/doctrine.yamlwith:But in practice, during configuration parsing, Symfony maps
~to[]and then performs an array merge instead of replacement, and thus your default values with a replica remain active...So you pretty much have to remove replicas from your default
doctrine.yamland configure it explicitly for all your environments that don't rely on this bundle.I'm not entirely sure how this can be worked around, to be honest.
About the only way to work around it that I can see is to have the StaticDriver somehow force Doctrine to return a connection to the primary specifically, maybe checking for when $connection is instanceof
\Doctrine\DBAL\Connections\PrimaryReadReplicaConnectionand callingensureConnectedToPrimary.The text was updated successfully, but these errors were encountered: