> On Nov. 21, 2016, 10:37 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java, > > lines 168-169 > > <https://reviews.apache.org/r/53685/diff/2/?file=1563432#file1563432line168> > > > > 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.
Did not plan to check in this after I used your recommendation to test if JMX ssl is enabled. > On Nov. 21, 2016, 10:37 p.m., Udo Kohlmeyer wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java, > > line 262 > > <https://reviews.apache.org/r/53685/diff/2/?file=1563434#file1563434line262> > > > > 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-` will do. Do we have one already? > On Nov. 21, 2016, 10:37 p.m., Udo Kohlmeyer wrote: > > geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java, > > lines 103-115 > > <https://reviews.apache.org/r/53685/diff/2/?file=1563435#file1563435line103> > > > > 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. the test is to test gfsh's connect command, which eventually uses jmx connection, not locator. > On Nov. 21, 2016, 10:37 p.m., Udo Kohlmeyer wrote: > > geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java, > > lines 119-142 > > <https://reviews.apache.org/r/53685/diff/2/?file=1563435#file1563435line119> > > > > I don't think you should ignore this test. Maybe mark them as flaky, > > but don't remove them. it's always failing if we don't put a long enough wait, not just flaky. - Jinmei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53685/#review156510 ----------------------------------------------------------- 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 > >
