One pool per node #19

shashitnak · 2023-03-15T12:39:35Z

Instead of creating one pool of Cluster connection, now creating one Cluster connection object which has a HashMap of Pool of each Node connection

ishan-juspay · 2023-04-10T06:35:37Z

src/Database/Redis/Cluster.hs

@@ -443,24 +443,24 @@ allMasterNodes (Connection nodeConns _ _ _ _) (ShardMap shardMap) =
 requestNode :: NodeConnection -> [[B.ByteString]] -> IO [Reply]
 requestNode (NodeConnection pool lastRecvRef nid) requests = do
    envTimeout <- round . (\x -> (x :: Time.NominalDiffTime) * 1000000) . realToFrac . fromMaybe (0.5 :: Double) . (>>= readMaybe) <$> lookupEnv "REDIS_REQUEST_NODE_TIMEOUT"
-    eresp <- race requestNodeImpl (threadDelay envTimeout)
+    eresp <- race (withResource pool requestNodeImpl) (threadDelay envTimeout)


@shashitnak We should actually call withResource even above the line where we throw the exception in case of a timeout. Otherwise we can run into a scenario where this connection times out and someother thread ends up using this connection and reading the stale data from last connection.

@ishan-juspay moved it upto requestNode. I think it will now handle the case that you are talking about

ishan-juspay · 2023-04-10T06:36:40Z

src/Database/Redis/Cluster.hs

@@ -443,24 +443,24 @@ allMasterNodes (Connection nodeConns _ _ _ _) (ShardMap shardMap) =
 requestNode :: NodeConnection -> [[B.ByteString]] -> IO [Reply]
 requestNode (NodeConnection pool lastRecvRef nid) requests = do
    envTimeout <- round . (\x -> (x :: Time.NominalDiffTime) * 1000000) . realToFrac . fromMaybe (0.5 :: Double) . (>>= readMaybe) <$> lookupEnv "REDIS_REQUEST_NODE_TIMEOUT"
-    eresp <- race requestNodeImpl (threadDelay envTimeout)
+    eresp <- race (withResource pool requestNodeImpl) (threadDelay envTimeout)
    case eresp of
        Left e -> return e
        Right _ -> putStrLn ("timeout happened" ++ show nid) *> throwIO NoNodeException


@shashitnak , lets create a custom exception for Timeout, we can name it RedisTimeoutException or something.
There might be other reasons for timeout apart from NoNode only

@ishan-juspay I actually thought about this. There is a test where an infinite script is sent to a node which causes it to timeout, which we aren't handling currently. So apart from NoNodeException, I'm thinking about adding a BusyNodeException

@ishan-juspay added a RedisTimeoutException type but it's still throwing NoNodeException wrapped in RedisTimeoutException for now. Will add the logic for figuring out the appropriate Exception later

ishan-juspay · 2023-04-10T06:43:47Z

src/Database/Redis/Cluster.hs

+        let connection = Connection nodeConns pipelineVar shardMapVar (CMD.newInfoMap commandInfos) isReadOnly
+        return connection
+    where
+    -- simpleNodeConnections :: ShardMap -> IO (HM.HashMap NodeID NodeConnection)


@shashitnak , can we remove these old commented code ? Doesn't look clean.

@ishan-juspay removed them while rebasing to the new master

ishan-juspay · 2023-04-10T07:45:17Z

src/Database/Redis/Cluster.hs

@@ -61,7 +62,7 @@ type IsReadOnly = Bool
 data Connection = Connection (HM.HashMap NodeID NodeConnection) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly


Since a pool is now per node, the map "HM.HashMap NodeID NodeConnection" is probably never going to update.
Can we find a way to update this mapping also? This will fail requests when we scale up Redis cluster where we won't be able to find the new NodeID in this map.

@ishan-juspay is it okay if we trigger the refresh logic at the point where some NodeId is not found in the map? We can have a Pool of a pair of HashMap and ShardMap and whenever some NodeId is not found in the HashMap, it will throw and Exception and a new constructor will be called for the Pair.

And this way, if some Resources are working with NodeId that they are able to find, then they will keep working without any problems.

We can have this HashMap as an MVar and update it whenever RefreshShardMap is called ? @aravindgopall

…Connection out of the Pool

aravindgopall · 2023-04-12T07:44:44Z

src/Database/Redis/Connection.hs

+          where
+          ctxToConn :: Cluster.NodeConnection -> IO (Maybe PP.Connection)
+          ctxToConn (Cluster.NodeConnection pool _ nid) = do
+            maybeConn <- try $ withResource pool PP.fromCtx


Do we have to remove the connection from pool if there is an exception? rather than catching with try and returning Nothing. Verify this once.

…lusterConnect function

shashitnak force-pushed the one-pool-per-node branch 2 times, most recently from 3b588c0 to 03fbbce Compare March 17, 2023 05:22

shashitnak force-pushed the one-pool-per-node branch from 41533fb to 8c39ee0 Compare April 7, 2023 13:26

ishan-juspay reviewed Apr 10, 2023

View reviewed changes

shashitnak force-pushed the one-pool-per-node branch from 8c39ee0 to 862480e Compare April 10, 2023 07:08

ishan-juspay reviewed Apr 10, 2023

View reviewed changes

shashitnak added 2 commits April 11, 2023 12:10

changing NodeConnection to Pool of NodeConnection and taking Cluster …

039a83d

…Connection out of the Pool

moving withResource call to the beginning of requestNode

bc79b73

shashitnak force-pushed the one-pool-per-node branch from 4b7190e to bc79b73 Compare April 11, 2023 07:00

aravindgopall reviewed Apr 12, 2023

View reviewed changes

moving NodeConnection HashMap in MVar and adding refreshCluster function

8545c0e

shashitnak force-pushed the one-pool-per-node branch from cab4f4a to 8545c0e Compare April 12, 2023 12:33

shashitnak added 3 commits April 12, 2023 18:36

refreshShardMap is now updating both MVar's and removing refreshCluster

0ee0bf0

dropping a NodeConnection from HashMap if it throws an exception in c…

47613cf

…lusterConnect function

only adding new nodes to hashmap and not changing the old ones

4362e64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

One pool per node #19

One pool per node #19

shashitnak commented Mar 15, 2023

ishan-juspay Apr 10, 2023

shashitnak Apr 10, 2023

ishan-juspay Apr 10, 2023

shashitnak Apr 10, 2023

shashitnak Apr 10, 2023

ishan-juspay Apr 10, 2023

shashitnak Apr 10, 2023

ishan-juspay Apr 10, 2023

shashitnak Apr 10, 2023

shashitnak Apr 10, 2023

ishan-juspay Apr 10, 2023 •

edited

Loading

aravindgopall Apr 12, 2023

		@@ -61,7 +62,7 @@ type IsReadOnly = Bool
		data Connection = Connection (HM.HashMap NodeID NodeConnection) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly

One pool per node #19

Are you sure you want to change the base?

One pool per node #19

Conversation

shashitnak commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishan-juspay Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishan-juspay Apr 10, 2023 •

edited

Loading