> 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-` > > Jinmei Liao wrote: > will do. Do we have one already?
maybe we can add one to the DistributionConfig. I wouldn't add it to the ConfigurationProperties, as that would be public. > 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. > > Jinmei Liao wrote: > the test is to test gfsh's connect command, which eventually uses jmx > connection, not locator. Then it might be that we need 2 tests... As we still need to test the the Locator SocketCreator sets up the correct SSL channel, not only the JMX channel. > 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. > > Jinmei Liao wrote: > it's always failing if we don't put a long enough wait, not just flaky. Maybe Awaitility can help us here.. But I would hate to think that we are losing existing test coverage because of this. > On Nov. 21, 2016, 10:37 p.m., Udo Kohlmeyer wrote: > > geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java, > > line 132 > > <https://reviews.apache.org/r/53685/diff/2/?file=1563435#file1563435line132> > > > > I don't think you should ignore this test. Maybe mark them as flaky, > > but don't remove them. Maybe Awaitility can help us here.. But I would hate to think that we are losing existing test coverage because of this. - Udo ----------------------------------------------------------- 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 > >
