> On March 21, 2017, 10:43 a.m., Jinmei Liao wrote: > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java > > Lines 72 (patched) > > <https://reviews.apache.org/r/57796/diff/1/?file=1670526#file1670526line72> > > > > I would hope we shouldn't need to manully set this in the test.
I was hoping not, also, but couldn't find another way to handle this. None of the parameters we are interested in are set in the system properties. For example `jmx-manager-port` is being set in the test, but it is not visible in `System.properties` when Pulse starts. > On March 21, 2017, 10:43 a.m., Jinmei Liao wrote: > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java > > Lines 159 (patched) > > <https://reviews.apache.org/r/57796/diff/1/?file=1670527#file1670527line161> > > > > Is this system proeprty set manually or by the locator/server when > > starting up the pulse service? It is set manually by the test. Prior to my change, Pulse only read from `pulse.properties` off of the classpath to set the `pulse.port` value or used the default. > On March 21, 2017, 10:43 a.m., Jinmei Liao wrote: > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java > > Lines 160 (patched) > > <https://reviews.apache.org/r/57796/diff/1/?file=1670527#file1670527line162> > > > > Which StringUtils is this? I believe we can use apache common's > > StringUtils.isBlank to do this. It is `org.apache.geode.tools.pulse.internal.util.StringUtils` which is already used heavily within `PulseAppListener`. Perhaps another ticket to remove this StringUtils in favor of apache commons? > On March 21, 2017, 10:43 a.m., Jinmei Liao wrote: > > geode-pulse/src/main/resources/pulse.properties > > Line 28 (original) > > <https://reviews.apache.org/r/57796/diff/1/?file=1670528#file1670528line28> > > > > I don't think we need to change this. This property file is only used > > when pulse is not in the embedded mode. Please look again at the diff. I've removed double-declared properties so your previous change is read. - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57796/#review169587 ----------------------------------------------------------- On March 21, 2017, 9:35 a.m., Kevin Duling wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57796/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 9:35 a.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > GEODE-2671: When a locator is started with a custom jmx-manager-port, the > embedded pulse server still tries to connect to jmx using 1099 > > > Diffs > ----- > > geode-assembly/build.gradle 1900896da96afdcc7c776f0cd98a2aea6840fb1d > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java > 57711258fbbc73570656e14ee8f05550ae32e891 > > geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java > 5408a5651774a63c16f27722c6ff7bda25cbaaa8 > geode-pulse/src/main/resources/pulse.properties > 878bc680bbcc4369eb2d3859c6377b8942bc89d7 > > > Diff: https://reviews.apache.org/r/57796/diff/1/ > > > Testing > ------- > > precheckin successful > > > Thanks, > > Kevin Duling > >
