----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61978/#review184098 -----------------------------------------------------------
The refactor is awesome! It would be nice though to have a test that ensures that we are in fact only incrementing counters for the correct operations. Would you mind either splitting out the refactor and making a separate commit for the fix that also contains a test or adding a test to this request? I'd personaly would very much prefer two separate commits for refactor and the change. geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java Lines 23 (patched) <https://reviews.apache.org/r/61978/#comment260113> This import is not used geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnectionTest.java Line 24 (original), 24 (patched) <https://reviews.apache.org/r/61978/#comment260114> I don't think we need to import the Acceptor any longer geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryTest.java Line 19 (original), 19 (patched) <https://reviews.apache.org/r/61978/#comment260115> acceptor import obsolete geode-protobuf/src/test/java/org/apache/geode/protocol/AuthenticationIntegrationTest.java Lines 105 (patched) <https://reviews.apache.org/r/61978/#comment260123> We are doing this for the RoundTripCacheConnectionJUnitTest already. So it's not surprising we need this here as well. However, in the Roundtrip test we are also calling `SocketCreatorFactory.close()`. I am unsure why we are doing that since that other test doesn't interact with the `SocketCreatorFactory`. Is that something we should be doing here as well? geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripLocatorConnectionJUnitTest.java Line 22 (original), 24 (patched) <https://reviews.apache.org/r/61978/#comment260116> acceptorImp and CommunicationMode aren't needed any longer - Alexander Murmann On Aug. 29, 2017, 9:39 p.m., Bruce Schuchardt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61978/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2017, 9:39 p.m.) > > > Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh > Khamesra, and Udo Kohlmeyer. > > > Bugs: GEODE-3059 > https://issues.apache.org/jira/browse/GEODE-3059 > > > Repository: geode > > > Description > ------- > > The stat should only be incremented/decremented for certain kinds of > connections. I've modified it to include protobuf connections. All of the > constant byte identifiers in Acceptor.java have been moved to an enum in > CommunicationMode.java. In that class I've added some "isa" checks to > replace the many big "if" checks for different kinds of connection modes. > > A new connection modes will henceforth need to be added to > CommunicationMode.java where the appropriate "isa" methods can be updated to > include the new mode. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java > dea8644bf6604a48b38e0f8a9fcfa48deb4b56c8 > > geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionImpl.java > 078844f7adc907761c8b0b1b5525874c8338144e > > geode-core/src/main/java/org/apache/geode/cache/server/internal/LoadMonitor.java > 1c571a924f5517f7ba1a04e216c3641f96f9ddc4 > > geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java > 83f87eebe3f7cbea628107078e1bafac478a0228 > geode-core/src/main/java/org/apache/geode/internal/cache/tier/Acceptor.java > e12a409bc556ab74718830ff8036edd6216ef53b > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/ClientHandShake.java > f7a39f3c86f261d76133b2bebb864f53be027f1a > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/CommunicationMode.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java > 2e33af897a852a2f397cf3c1d7f3b20ae3b1d69f > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientNotifier.java > e2612fc45ccf59ee64654380b771835415780f9d > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientHealthMonitor.java > e877852a42009c0cadeaa0e69516bdbcd84e6bd4 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ConnectionListener.java > 104d88abc99d3b329834f53d558b4684b6f9c226 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ConnectionListenerAdapter.java > 7476b4fba5def66c52b1fa26fb3f0f2e4c63fd17 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/HandShake.java > 690fd83971d9ac9f8ecedf9b89acb7ade887f38e > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java > 6f56e85382ded1668bc51f6c0f2cc4990a493750 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java > 00e8b8880a5f336bb82c578f29acaf256187bb5c > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerHandShakeProcessor.java > 47e6f3d0bd9cab249e8f92398b116c6111b0e60e > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnectionTest.java > ea0001867e5a22025f4dbca4ead172ff25f2af4d > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactoryTest.java > cffa05fa49de58187eaab5bb10c7f2a6ed3bf0f5 > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionTest.java > 2aa89954659594428ae3f6ed6d7146261d518203 > > geode-protobuf/src/test/java/org/apache/geode/protocol/AuthenticationIntegrationTest.java > 122b8e3ba7ecab407fdedf6be35153a1def728ff > > geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripLocatorConnectionJUnitTest.java > 7ee307b4068247245fc02ab1ae7266c7e2e385c3 > > > Diff: https://reviews.apache.org/r/61978/diff/1/ > > > Testing > ------- > > precheckin is running now > > > Thanks, > > Bruce Schuchardt > >
