Deprecate master/slave terminology and underlying feature by greg0ire · Pull Request #4055 · doctrine/dbal

greg0ire · 2020-06-07T08:02:12Z

Q	A
Type	improvement
BC Break	no
Fixed issues	#4052

Summary

MasterSlaveConnection is left untouched, deprecated, copy/pasted/improved
Alternative to #4054

Targeting 2.11.x because of the deprecation.

greg0ire · 2020-06-07T08:04:04Z

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

+    /**
+     * Connects to a specific connection.
+     */
+    private function connectTo(string $connectionName): DriverConnection


If you think somebody relies on connectTo or chooseConnectionConfiguration, we could make them public. Doing so after this is merged and release would not be a BC break though.

If somebody needs to choose the connection explicitly, they should operate those connections manually w/o this wrapper.

Why? And who would that somebody be? This method is private, so it can't be used by somebody outside this class if that's who you were thinking.

If you think somebody relies on connectTo or chooseConnectionConfiguration, we could make them public.

It was the answer to your comment. I don't think that this should be made part of the public API.

SenseException · 2020-06-07T14:22:55Z

lib/Doctrine/DBAL/DriverManager.php

        $params = self::parseDatabaseUrl($params);

-        // URL support for MasterSlaveConnection
+        // @todo: deprecated, notice thrown by connection constructor


Will the Connection constructor get a deprecation message in this PR?

Ah right that's a part I forgot to backport from #4054

Unsure about the @todo part, I copied it from #4054, is this a way we have to remember to remove things on 3.0.x?

I don't know. I thought @deprecated and/or trigger_error() is usually in use.

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

codecov-commenter · 2020-06-07T15:39:41Z

Codecov Report

Merging #4055 into 2.11.x will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             2.11.x    #4055   +/-   ##
=========================================
  Coverage     71.02%   71.02%           
  Complexity     5261     5261           
=========================================
  Files           217      217           
  Lines         13355    13358    +3     
=========================================
+ Hits           9485     9488    +3     
  Misses         3870     3870

Impacted Files	Coverage Δ	Complexity Δ
...octrine/DBAL/Connections/MasterSlaveConnection.php	`67.85% <100.00%> (+0.88%)`	`42.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cfb57d...5149e52. Read the comment docs.

morozov

API-wise looks good. We should be able to address the implementation details issues later.

morozov · 2020-06-07T18:38:46Z

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

+     *
+     * @var DriverConnection[]|null[]
+     */
+    private $connections = ['primary' => null, 'replica' => null];


As far as I understand, it's one primary and many replicas. Why do we need to keep them under one array with magic keys? It's fine if we don't resolve it in this ticket.

I can resolve it in this ticket, I think that will be the easiest part TBH :P

In fact it's just the one replica (picked at random), as stated in the comment above. Your comment still applies though.

morozov · 2020-06-07T18:40:23Z

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

+    /**
+     * Connects to a specific connection.
+     */
+    private function connectTo(string $connectionName): DriverConnection


If somebody needs to choose the connection explicitly, they should operate those connections manually w/o this wrapper.

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

morozov · 2020-06-07T19:20:54Z

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

+        }
+
+
+        parent::__construct($params, $driver, $config, $eventManager);


This is another place where my eyes are bleeding. The parent class doesn't understand the "replica" key but for some reason, we're passing it there. Shouldn't the concept of multiple connections be encapsulated to this very class?

🤔 uh yeah that's nasty. Are you suggesting unset-ing replica and primary, storing them in some other property, or not calling the parent constructor altogether (and then maybe stop extending Connection??)

We cannot avoid extending the connection because there's no DBAL-level connection interface to implement. Unsetting and managing the replica parameters within the class would help migrate to composition later.

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

morozov · 2020-06-07T19:30:47Z

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

+
+    public function query(): Statement
+    {
+        $this->switchToPrimary();


And it's even worse here. Why are we switching to the primary endpoint to execute a query?

I won't lie, I have no idea…

the query function can be used for any kind of SQL, including write operations. This cannot be detected without parsing the SQL statement, which is a rabbit hole. This is why for safety reason the connectoin assumes that when using $connection->query(), you might be performing a write operation.

This is why the developer should be in control of which connection to use for a given statement. Not the abstraction layer.

greg0ire · 2020-06-07T19:33:09Z

@morozov this looks nasty too:

dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php

Lines 34 to 41 in 0ad4c16

    
            * ATTENTION: You can write to the slave with this connection if you execute a write query without 
        
            * opening up a transaction. For example: 
        
            * 
        
            *      $conn = DriverManager::getConnection(...); 
        
            *      $conn->executeQuery("DELETE FROM table"); 
        
            * 
        
            * Be aware that Connection#executeQuery is a method specifically for READ 
        
            * operations only.

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

morozov · 2020-06-07T19:39:36Z

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

+
+        $driverOptions = $params['driverOptions'] ?? [];
+
+        $connectionParams = $this->chooseConnectionConfiguration($connectionName, $params);


So does this mean that every time we "switch" between the primary and replica endpoints, the replica will be randomly chosen and reconnected to while the old replica connection will be closed? Even if there's only one replica configured. What's the rationale behind this behavior?

🤔 we are not supposed to switch from the primary to the replica, are we?

dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php

Line 31 in 0ad4c16

* 3. If master was picked once during the lifetime of the connection it will always get picked afterwards.

This is really questionable behavior. If I insert a "user logged in" event in one table and then query a heavy report from the other, why should the report use the master connection?

The step from primary to replica is not really a common use-case, but what you describe could be an example. If the application does a single write early in the request and you can isolate that and don't need to care about replication lag with this write, then you could switch back to a replica. The idea would be:

$connection->connect('primary'); $connection->executeUpdate("...."); $connection->connect("replica");

Why not just do $primaryConnection->executeUpdate(); $replicaConnection->doWhatever();? Why keep one connection as a shared state and switch between the inner connections implicitly?

The answer is probably "convenience" (at the cost of some usefulness). Like always with such things ;)

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

morozov · 2020-06-07T19:59:52Z

Given the number of WTFs that this code produces, I'd personally prefer to just deprecate it for now. If we don't manage to implement a proper replacement (and given all the possible variations of the behavior, I don't think we should), the old code could be just copy/pasted in the project that needs this functionality. But looking at the behaviors that it implements, I hope nobody in their right mind uses it in production.

greg0ire · 2020-06-07T20:03:39Z

@morozov I agree. In my company, we have a RW endpoint and an RO endpoint, we just created 2 connections, and don't use this class, and it works just fine. These features could be brought back at some point with a brand new design, that would have to avoid this unholy mix of composition and inheritance.

morozov · 2020-06-07T20:05:30Z

In my company, we have a RW endpoint and an RO endpoint, we just created 2 connections, and don't use this class, and it works just fine.

This is how I see it should be implemented. The existing implementation has too many questionable assumptions to be the one and the only that is shipped with DBAL.

greg0ire · 2020-06-07T20:13:25Z

Ah I need to add an upgrade note for this.

morozov · 2020-06-07T20:41:26Z

UPGRADE.md

+## Deprecated `MasterSlaveConnection`
+
+The naming is offensive, the implementation very questionable. Use regular
+connections and switch between them explicitely.


Should it be “explicitly”? I'd also avoid “switch” — it implies shared state. Let's use “Use each endpoint connection explicitly”.

Not only is the terminology needlessly hurtful, the design is a very leaky abstraction, and users are better off using regular connections and managing them themselves explicitly.

beberlei · 2020-06-09T10:54:37Z

Closing in favor of #4054

greg0ire commented Jun 7, 2020

View reviewed changes

greg0ire mentioned this pull request Jun 7, 2020

[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection #4054

Merged

greg0ire requested review from beberlei and morozov June 7, 2020 08:27

greg0ire closed this Jun 7, 2020

greg0ire reopened this Jun 7, 2020

albe previously approved these changes Jun 7, 2020

View reviewed changes

SenseException reviewed Jun 7, 2020

View reviewed changes

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

greg0ire dismissed albe’s stale review via 52da3f5 June 7, 2020 15:28

greg0ire force-pushed the deprecate-master-slave branch from 755b058 to 52da3f5 Compare June 7, 2020 15:28

morozov reviewed Jun 7, 2020

View reviewed changes

greg0ire force-pushed the deprecate-master-slave branch from 02971b3 to 0f368ee Compare June 7, 2020 19:19

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

morozov reviewed Jun 7, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

greg0ire force-pushed the deprecate-master-slave branch from 0f368ee to e6a7703 Compare June 7, 2020 20:13

greg0ire force-pushed the deprecate-master-slave branch from e6a7703 to 552eb88 Compare June 7, 2020 20:15

greg0ire changed the title ~~Deprecate master/slave terminology~~ Deprecate master/slave terminology and underlying feature Jun 7, 2020

morozov reviewed Jun 7, 2020

View reviewed changes

greg0ire force-pushed the deprecate-master-slave branch from 552eb88 to 0d621ba Compare June 7, 2020 20:45

morozov previously approved these changes Jun 7, 2020

View reviewed changes

Deprecate MasterSlaveConnection class

5149e52

Not only is the terminology needlessly hurtful, the design is a very leaky abstraction, and users are better off using regular connections and managing them themselves explicitly.

greg0ire dismissed morozov’s stale review via 5149e52 June 7, 2020 21:11

greg0ire force-pushed the deprecate-master-slave branch from 0d621ba to 5149e52 Compare June 7, 2020 21:11

morozov approved these changes Jun 7, 2020

View reviewed changes

beberlei closed this Jun 9, 2020

greg0ire deleted the deprecate-master-slave branch June 9, 2020 11:36

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2022

		}


		parent::__construct($params, $driver, $config, $eventManager);


		$driverOptions = $params['driverOptions'] ?? [];

		$connectionParams = $this->chooseConnectionConfiguration($connectionName, $params);

Uh oh!

Conversation

greg0ire commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Jun 7, 2020

Uh oh!

Uh oh!

morozov Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

greg0ire commented Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jun 7, 2020 •

edited

Loading

greg0ire Jun 7, 2020 •

edited

Loading

morozov Jun 7, 2020 •

edited

Loading

greg0ire Jun 7, 2020 •

edited

Loading

morozov Jun 7, 2020 •

edited

Loading

greg0ire Jun 7, 2020 •

edited

Loading

morozov Jun 7, 2020 •

edited

Loading

morozov commented Jun 7, 2020 •

edited

Loading

morozov Jun 7, 2020 •

edited

Loading