----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53685/#review156510 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java (lines 163 - 164) <https://reviews.apache.org/r/53685/#comment226693> I don't believe this should be a public static method. There should be no reason to expose this publically. The SSLConfigurationFactory is only there to provide you with the SSLConfig per component. Also, we don't pass in the distribution config. This method has to use the DistributionConfig that was assigned to it. Otherwise one might be inclined to pass in arbitrary DistributionConfiguration. geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java (line 243) <https://reviews.apache.org/r/53685/#comment226694> I think you should rather put this as a static reference, it becomes a lot harder to refactor if one has to search for all string the look like `ssl-` geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (lines 101 - 112) <https://reviews.apache.org/r/53685/#comment226697> Why would we have removed the test to test the SSL communication with the locator? If you need a test to test JMX add a test for that. geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (lines 115 - 138) <https://reviews.apache.org/r/53685/#comment226695> I don't think you should ignore this test. Maybe mark them as flaky, but don't remove them. geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (line 128) <https://reviews.apache.org/r/53685/#comment226696> I don't think you should ignore this test. Maybe mark them as flaky, but don't remove them. - Udo Kohlmeyer On Nov. 14, 2016, 8:56 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53685/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2016, 8:56 p.m.) > > > Review request for geode, Kevin Duling, Kirk Lund, and Udo Kohlmeyer. > > > Repository: geode > > > Description > ------- > > * add the "disconnect" command when we close the GfshShellConnectionRule so > that no heartbeat thread is left to pollute other tests. > * Fix the test so that it truely test the jmx ssl connection, and fix the > configuration mishaps. > * The above fix revealed another bug (GEODE-2099: race condition), ignored > the other two tests for now. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java > 76fb04169f06f82022858cbd0a03eadac8a42ef6 > > geode-core/src/main/java/org/apache/geode/management/internal/JmxManagerAdvisee.java > 0467c486a0e343bf745fe743172ed4fce750c5b4 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java > 456f76bb5592b000c75fe733553219c09d1b5381 > > geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java > b6afcca7cf8a27c2ad90d6ea570755a63ac0e89b > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 44da08be416240f49cbe9dfb8653f41eaea8777e > > Diff: https://reviews.apache.org/r/53685/diff/ > > > Testing > ------- > > > Thanks, > > Jinmei Liao > >
