> 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.
>
> Kevin Duling wrote:
> 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.
>
> Jinmei Liao wrote:
> how is this IsEmbededMode system property set? I would think if we are
> starting pulse in the managementAgent, we should set this system property.
`ManagementAgent` has `System.setProperty(PULSE_EMBEDDED_PROP, "true");` at
line within `if (agentUtil.isWebApplicationAvailable(gemfireWar, pulseWar,
gemfireAPIWar)) {`
> 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?
>
> Kevin Duling wrote:
> 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.
>
> Jinmei Liao wrote:
> So when a user starts a locator with jmx-manager-port other than 1099, he
> still would have problem having pulse connect to this locator?
He would have to set `pulse.port` to match `jmx-manager-port`. Pulse has a lot
of different properties, such as `pulse.host`, `pulse.embedded`,
`pulse.embedded.sqlf`, etc. They're laid out in the `pulse.properties` file.
One could argue that pulse should look for `jmx-manager-port` but then rules
will have to be defined for when someone specifies both properties. Which one
wins?
> 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.
>
> Kevin Duling wrote:
> 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?
>
> Jinmei Liao wrote:
> We should really get rid of those random StringUtils to use a standard
> library. Agree another ticket is probably a good idea if you can get to it.
Agreed, I'll open another ticket to remove it.
- 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
>
>
