One pool per node test #24

shashitnak · 2023-04-26T12:16:01Z

This PR contains latest changes that were made to fix the errors that were discovered during Chaos testing following this doc https://docs.google.com/spreadsheets/d/1u02O5RDdAGgQOBDsNjEsFflWNfPKkP8dZvCVXm4jZC4/edit#gid=0

Cluster Support

…ForRefreshShardMap Added retry logic for refreshShardMap

MissingNodeException bugfix

throwing TimeoutException for timeouts

…Connection out of the Pool

…lusterConnect function

…ly new HashMap on refresh

ishan-juspay · 2023-04-28T19:01:44Z

src/Database/Redis/Cluster.hs

-                  "TimeoutException" -> throwIO TimeoutException
-                  _ -> executeRequests (getRandomConnection cc conn) r
+              Left (err :: SomeException) ->
+                if isPrefixOf "TimeoutException" (displayException err)


Do we need to match string in case of exception ? Can't we have a type of exception and pattern match it ?

ishan-juspay · 2023-04-28T19:03:13Z

src/Database/Redis/Connection.hs

    pipelineConn <- PP.fromCtx ctx
-    raceResult <- race (threadDelay (10^(3 :: Int))) (try $ refreshShardMapWithConn pipelineConn True) -- racing with delay of 1 ms 
+    envTimeout <- fromMaybe (10 ^ (3 :: Int)) . (>>= readMaybe) <$> lookupEnv "REDIS_CLUSTER_SLOTS_TIMEOUT"


Should we put these timeouts default in a single Config file within Hedis ? Having them distributed now is looking untidy.

ishan-juspay · 2023-04-28T19:09:52Z

src/Database/Redis/Cluster.hs

@@ -453,34 +454,33 @@ allMasterNodes (Connection nodeConns _ _ _ _) (ShardMap shardMap) =
    onlyMasterNodes = (\(Shard master _) -> master) <$> nub (IntMap.elems shardMap)

 requestNode :: NodeConnection -> [[B.ByteString]] -> IO [Reply]
-requestNode (NodeConnection ctx lastRecvRef _) requests = do
+requestNode (NodeConnection pool lastRecvRef _) requests = do
    envTimeout <- round . (\x -> (x :: Time.NominalDiffTime) * 1000000) . realToFrac . fromMaybe (0.5 :: Double) . (>>= readMaybe) <$> lookupEnv "REDIS_REQUEST_NODE_TIMEOUT"


Lets move these default values and environment variables into a single Config file. @shashitnak

ishan-juspay · 2023-04-28T19:12:59Z

src/Database/Redis/Connection.hs

+    let (Cluster.NodeConnection pool _ _) = nodeConnsList !! selectedIdx
+    withResource pool $ \ctx -> do
+        pipelineConn <- PP.fromCtx ctx
+        envTimeout <- fromMaybe (10 ^ (3 :: Int)) . (>>= readMaybe) <$> lookupEnv "REDIS_CLUSTER_SLOTS_TIMEOUT"


Same comment, lets combine all such configs in a single place to be more readable / trackable.

ishan-juspay · 2023-05-02T04:47:17Z

src/Database/Redis/Cluster.hs

@@ -60,7 +66,7 @@ import qualified Database.Redis.Cluster.Command as CMD
 -- | 'NodeConnection's, a 'Pipeline', and a 'ShardMap'
 type IsReadOnly = Bool

-data Connection = Connection (HM.HashMap NodeID NodeConnection) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly
+data Connection = Connection (MVar NodeConnectionMap) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly TcpInfo


@shashitnak @aravindgopall shouldn't we have this as MVar ShardMap NodeConnectionMap ? That would make them coupled which ideally should be the case? Would it be too many changes in case we do so?

ishan-juspay · 2023-05-02T04:50:34Z

src/Database/Redis/Cluster.hs

+    { connectAuth           :: Maybe B.ByteString
+    , connectTLSParams      :: Maybe ClientParams
+    , idleTime              :: Time.NominalDiffTime
+    , maxResources          :: Int


Should it be renamed as TcpPoolInfo?

ishan-juspay · 2023-05-02T08:01:51Z

src/Database/Redis/Cluster.hs

@@ -66,7 +69,7 @@ import Network.TLS (ClientParams)
 -- | 'NodeConnection's, a 'Pipeline', and a 'ShardMap'
 type IsReadOnly = Bool

-data Connection = Connection (MVar NodeConnectionMap) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly TcpInfo
+data Connection = Connection (TMVar NodeConnectionMap) (MVar Pipeline) (TMVar (ShardMap, Time.UTCTime)) CMD.InfoMap IsReadOnly TcpInfo


Lets create a typeAliase here and name it "LastUpdatedAt" and use it here.

ishan-juspay · 2023-05-02T09:01:47Z

src/Database/Redis/Cluster.hs

@@ -66,7 +69,7 @@ import Network.TLS (ClientParams)
 -- | 'NodeConnection's, a 'Pipeline', and a 'ShardMap'
 type IsReadOnly = Bool

-data Connection = Connection (MVar NodeConnectionMap) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly TcpInfo
+data Connection = Connection (TMVar NodeConnectionMap) (MVar Pipeline) (TMVar (ShardMap, Time.UTCTime)) CMD.InfoMap IsReadOnly TcpInfo


Lets use a type alias here for Time.UTCTime , use LastUpdatedAt maybe ?

ishan-juspay · 2023-05-02T09:02:26Z

src/Database/Redis/Cluster.hs

 connect withAuth commandInfos shardMapVar isReadOnly refreshShardMap (tcpInfo@TcpInfo{ timeoutOpt, maxResources, idleTime }) = do
-        shardMap <- readMVar shardMapVar
+        shardMap <- atomically $ readTMVar shardMapVar


Ideally we should aim for not blocking all threads using Atomically here. We need to do a lock free read on shardMap otherwise this can be a perf bottleneck.

I am not sure this understanding is right. But @shashitnak you can use IO version of functions like readTMVarIO rather than atomically to convert from STM to IO.

ishan-juspay · 2023-05-02T09:03:26Z

src/Database/Redis/Cluster.hs

                Just allNodes' -> return allNodes'

 allMasterNodes :: Connection -> ShardMap -> IO (Maybe [NodeConnection])
 allMasterNodes (Connection nodeConnsVar _ _ _ _ _) (ShardMap shardMap) = do
-    nodeConns <- readMVar nodeConnsVar
+    nodeConns <- atomically $ readTMVar nodeConnsVar


Idea should be:
All reads should be lock free. Only during an update, we should acquire a lock (make other threads wait) and update the MVar.

All reads should be lock free
With (T)MVar this might never be true.

aravindgopall · 2023-05-02T18:30:58Z

@shashitnak can we also add the time column in the sheet.

aravindgopall · 2023-05-02T18:15:32Z

src/Database/Redis/Cluster.hs

@@ -66,7 +69,7 @@ import Network.TLS (ClientParams)
 -- | 'NodeConnection's, a 'Pipeline', and a 'ShardMap'
 type IsReadOnly = Bool

-data Connection = Connection (MVar NodeConnectionMap) (MVar Pipeline) (MVar ShardMap) CMD.InfoMap IsReadOnly TcpInfo
+data Connection = Connection (TMVar NodeConnectionMap) (MVar Pipeline) (TMVar (ShardMap, Time.UTCTime)) CMD.InfoMap IsReadOnly TcpInfo


aravindgopall · 2023-05-02T18:23:01Z

src/Database/Redis/Cluster.hs

                Just allNodes' -> return allNodes'

 allMasterNodes :: Connection -> ShardMap -> IO (Maybe [NodeConnection])
 allMasterNodes (Connection nodeConnsVar _ _ _ _ _) (ShardMap shardMap) = do
-    nodeConns <- readMVar nodeConnsVar
+    nodeConns <- atomically $ readTMVar nodeConnsVar


All reads should be lock free
With (T)MVar this might never be true.

aravindgopall · 2023-05-02T18:26:07Z

src/Database/Redis/Connection.hs

+    putStrLn "Refresh called."
+    (oldShardMap, lastUpdated) <- atomically $ readTMVar shardMapVar
+    now <- Time.getCurrentTime
+    if Time.diffUTCTime now lastUpdated <= shardMapRefreshDelta


When this will be the case? We can make MVar as empty to implement multiple threads not invoking the refreshShardMap at same time rather than maintain this complex logic of using timestamps.

@aravindgopall problem here is: We want to make reads as lock free. So basically when a thread reads the MVar and till the time it gets an exception because of topology change and tries to update the MVar, other threads might also come and try to update this.
To avoid multiple threads updating the shardMap at the same time this is being done. There should be a cleaner way of doing this but I felt this is okay to unblock this commit merge ?

Anyways, the timestamp check should only happen in case of exceptions because of topology change, so not much of a perf difference too.

aravindgopall · 2023-05-02T18:29:33Z

src/Database/Redis/Cluster.hs

 connect withAuth commandInfos shardMapVar isReadOnly refreshShardMap (tcpInfo@TcpInfo{ timeoutOpt, maxResources, idleTime }) = do
-        shardMap <- readMVar shardMapVar
+        shardMap <- atomically $ readTMVar shardMapVar


I am not sure this understanding is right. But @shashitnak you can use IO version of functions like readTMVarIO rather than atomically to convert from STM to IO.

shashitnak · 2023-05-05T13:59:18Z

@ishan-juspay @aravindgopall sorry for not replying as I didn't visit this PR since a few days. I moved to TMVar because I wasn't completely aware of how MVar's worked so I thought it wouldn't be possible to modify two MVar's without causing race condition or deadlock. But I managed to fix it by using the method that @aravindgopall mentioned. By emptying the MVar as soon as the first thread gets to it and other's will just return readMVar, which will block until the first one will update the shardMap and put it back. I did chaos testing for this change and they are passing

aravindgopall

I feel changes are good to go.

aravindgopall · 2023-05-11T01:32:45Z

src/Database/Redis/Connection.hs

-refreshShardMap (Cluster.Connection nodeConns _ _ _ _) =
-    refreshShardMapWithNodeConn (head $ HM.elems nodeConns)
+refreshShardMap (Cluster.Connection nodeConnsVar _ shardMapVar _ _ Cluster.TcpInfo { idleTime, maxResources, timeoutOpt, connectAuth, connectTLSParams }) = do
+    putStrLn "ShardMap Refreshed."


let's remove comments.

neeraj97 · 2023-05-15T13:48:39Z

src/Database/Redis/Cluster.hs


 -- Add a request to the current pipeline for this connection. The pipeline will
 -- be executed implicitly as soon as any result returned from this function is
 -- evaluated.
 requestPipelined :: IO ShardMap -> Connection -> [B.ByteString] -> IO Reply
-requestPipelined refreshAction conn@(Connection _ pipelineVar shardMapVar _ _) nextRequest = modifyMVar pipelineVar $ \(Pipeline stateVar) -> do
+requestPipelined refreshShardmapAction conn@(Connection _ pipelineVar shardMapVar _ _ _) nextRequest = modifyMVar pipelineVar $ \(Pipeline stateVar) -> do
    (newStateVar, repliesIndex) <- hasLocked $ modifyMVar stateVar $ \case


@shashitnak This global pipelineVar will cause an issue during multi exec.
To replicate this add threadDelay like 1 min after multi in multiExec function.

Run a multi exec command.

Run in parallel some other command that will not fall into same hashslot (if possible different node in cluster) of above multiexec cmds.

cc. @aravindgopall

or else You can try with ghci

shashitnak · 2023-05-16T15:38:47Z

@ishan-juspay @neeraj97 @aravindgopall moved pipelineVar inside the NodeConnection object and that IORef inside the Pool. Since every node has it's own pipeline now, there are some issues in how the replies are being sent to the caller. I think the problem is in cases where some request has to go to all or multiple the nodes. I get a list of reply's inside requestPipelined but I have to return only one. For now I am returning the first one which I think is causing many of the test cases to fail.

ishan-juspay · 2023-05-22T03:10:17Z

src/Database/Redis/Cluster.hs


 -- | A connection to a single node in the cluster, similar to 'ProtocolPipelining.Connection'
-data NodeConnection = NodeConnection (Pool CC.ConnectionContext) (IOR.IORef (Maybe B.ByteString)) NodeID
+data NodeConnection = NodeConnection (Pool SingleNodeConnection) (MVar Pipeline) NodeID


Pipeline MVar needs to be part of Pool @shashitnak

ishan-juspay · 2023-06-05T17:23:05Z

src/Database/Redis/Cluster.hs

-data NodeConnection = NodeConnection (Pool CC.ConnectionContext) (IOR.IORef (Maybe B.ByteString)) NodeID
+data NodeConnection = NodeConnection (Pool NodeResource) NodeID
+
+data NodeResource = NodeResource CC.ConnectionContext (IOR.IORef (Maybe B.ByteString))


@shashitnak lets use a newtype here, data declaration are lesser performant than newtype since newtype is transparent at runtime.

@ishan-juspay converted NodeResource to newtype. since newtype can only take one parameter so I made the two parameters into a tuple

neeraj97 · 2023-06-12T08:18:37Z

src/Database/Redis/Connection.hs

@@ -117,6 +128,9 @@ defaultConnectInfo = ConnInfo
    , connectTLSParams      = Nothing
    }

+shardMapRefreshDelta :: Time.NominalDiffTime
+shardMapRefreshDelta = Time.secondsToNominalDiffTime 60
+


Where is this being used?

@neeraj97 this was used for a change that I added briefly for fixing the dead lock which was caused due to refreshShardMap function. Whenever a refresh was called by some thread, no other call to refreshShardMap would do the refresh until shardMapRefreshDelta time has passed. I shifted to using a combination of tryTakeMVar and the blocking nature of readMVar to achive this but then forgot to remove this. Not sure why haskell compiler didn't give unused warning even though this is not being used anywhere

I have removed this now

neeraj97 · 2023-06-12T11:23:51Z

src/Database/Redis/Connection.hs

+                    Nothing -> return Nothing
+                ) nodeConnsPair
+          let newMap = HM.fromList $ catMaybes workingNodes
+          _ <- forkIO $ modifyMVar_ nodeMapVar (\_ -> return newMap)


Why forkIO?

@neeraj97 I don't remember exactly but I think this could be amongst the changes that I was trying for fixing that deadlock in refreshShardMap. I removed this now

neeraj97 · 2023-06-12T11:58:57Z

src/Database/Redis/Connection.hs

+          workingNodes <- mapM (\(maybeConn, nodeConn) -> case maybeConn of
+                    Just conn -> do
+                        when readOnlyConnection $ do
+                            PP.beginReceiving conn


@shashitnak Is this for readOnly used for reading from slaves?
If so should we remove this logic or build the correct logic for supporting so.
@ishan-juspay

@neeraj97 @ishan-juspay removed clusterConnect function entirely

neeraj97 · 2023-06-12T12:27:24Z

src/Database/Redis/Cluster.hs

@@ -238,19 +284,34 @@ rawResponse (CompletedRequest _ _ r) = r
 -- cluster reconfiguration events, which should be rare.
 evaluatePipeline :: MVar ShardMap -> IO ShardMap -> Connection -> [[B.ByteString]] -> IO [Reply]
 evaluatePipeline shardMapVar refreshShardmapAction conn requests = do
-        shardMap <- hasLocked $ readMVar shardMapVar
-        requestsByNode <- getRequestsByNode shardMap
+        shardMap <- readMVar shardMapVar


why hasLocked has been removed?
It would allow us to throw an exception if things are blocked indefinitely on MVar

@neeraj97 added this back now

aravindgopall and others added 16 commits February 7, 2023 12:38

Merge pull request juspay#4 from juspay/cluster

10e07f3

Cluster Support

Added retry logic for refreshShardMap

910977c

MissingNodeException bugfix

638e652

Merge pull request juspay#21 from gupta-ujjwal/feature/NodeRetryLogic…

3f49188

…ForRefreshShardMap Added retry logic for refreshShardMap

Merge pull request juspay#22 from imviv3kshukla/fix/MissingNodeException

a21fa1b

MissingNodeException bugfix

throwing TimeoutException for timeouts

974d85e

randomly choosing node for refreshShardMap

fc3b373

using fromException instead of displayException

b7aea75

Merge pull request juspay#23 from Candyman770/fix/timeoutException

22d8146

throwing TimeoutException for timeouts

changing NodeConnection to Pool of NodeConnection and taking Cluster …

039a83d

…Connection out of the Pool

moving withResource call to the beginning of requestNode

bc79b73

moving NodeConnection HashMap in MVar and adding refreshCluster function

8545c0e

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

saving a timestamp for last ShardMap refresh Time and creating entire…

4fd99de

…ly new HashMap on refresh

ishan-juspay reviewed Apr 28, 2023

View reviewed changes

ishan-juspay reviewed May 2, 2023

View reviewed changes

aravindgopall reviewed May 2, 2023

View reviewed changes

moving back to MVar's and fixing deadlock

e22fbee

aravindgopall reviewed May 11, 2023

View reviewed changes

shashitnak marked this pull request as ready for review May 15, 2023 11:48

neeraj97 reviewed May 15, 2023

View reviewed changes

shashitnak marked this pull request as draft May 16, 2023 15:29

ishan-juspay reviewed May 22, 2023

View reviewed changes

shashitnak added 2 commits June 5, 2023 16:38

moving Pipeline from Connection object to ClusterEnv

883936f

moving IORef inside Pool

7427b2f

shashitnak force-pushed the one-pool-per-node-test branch from 73654c1 to 7427b2f Compare June 5, 2023 12:10

ishan-juspay reviewed Jun 5, 2023

View reviewed changes

changing NodeResource from data to newtype

1230e4c

neeraj97 reviewed Jun 12, 2023

View reviewed changes

shashitnak added 3 commits June 20, 2023 14:21

addressing pr comments

8f27a40

fixing redudant import error due to previous change

5e9065c

removing clusterConnect function

7eed199

One pool per node test #24

Are you sure you want to change the base?

One pool per node test #24

Conversation

shashitnak commented Apr 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindgopall commented May 2, 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

shashitnak commented May 5, 2023

aravindgopall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neeraj97 May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashitnak commented May 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neeraj97 May 15, 2023 •

edited

Loading