[GitHub] [geode-native] gaussianrecurrence edited a comment on pull request #660: GEODE-8553: Fix inter-locks in ThinClientLocator

[GitBox <gi...@apache.org>]

gaussianrecurrence edited a comment on pull request #660:
URL: https://github.com/apache/geode-native/pull/660#issuecomment-702736992


   Hi again,
   
   Regarding the test proposal, I've got an idea I would like to share with you in order to get your opinion on the matter.
   
   The purpose of this PR is avoid that whenever a thread is connecting to a locator or performing a request, other threads accesing the pool locator **don't get locked**.
   
   Therefore what I thought is to:
   - Introduce a new abstract class **ConnectorFactory**, which basically creates a new **Connector**.
   - Create a **ConnectorFactory** implementation called **TcpConnFactory** which basically creates **TcpConn**/**TcpSslConn**. This itself would also help reduced duplicated code, as connections creation is duplicated both in **TcrConnection** and **ThinClientLocatorHelper**.
   - PoolFactory would be the one responsible for setting the instance of ConnectorFactory into the created pool, adding therefore a method to set ConnectorFactory instance in there.
   - ConnectorFactory should be then accesible throught the Pool.
   - Create a Google Mock for ConnectorFactory.
   - Create a Google Mock for Connector.
   
   **PST.** While seeking a solution for the test proposal I think I've noticed something: "Is it possible that many of the factory class existing in NC, should be instead builders?"
   
   Ok, so having set all the above scaffolding, then the idea would be to create a ThinClientLocatorHelper TS which tries to perform requests by multiple threads, and having the ability to control whats returned by the connections and how much time is spent on a connection operation, you can therefore check that if one thread is waiting for an operation to complete, the other thread can inmmediatly return.
   
   An pseudo-code example would be the following:
   
   **testGetAllServerWhileUpdating**
   `
   createCache();
   createPoolFactoryWithCustomConnectorFactory();
   
   updateT = createUpdateThread(connector_sleep_time=10s);
   getAllServersT = createGetAllServersThread(connector_sleep_time=0s);
   
   ASSERT_EQ(getAllServersT.wait_for(1s), std::future_state::ready);
   `
   
   ---
   So want do you think about it?
   What I don't know is where would I put the test, for that I might need a hand from your side :)
   
   BR
   /Mario


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org