You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kevin Duling <kd...@pivotal.io> on 2017/03/20 22:06:51 UTC
Review Request 57796: 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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/
-----------------------------------------------------------
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 running
Thanks,
Kevin Duling
Re: Review Request 57796: 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
Posted by Jinmei Liao <ji...@pivotal.io>.
> On March 21, 2017, 5:43 p.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.
>
> Kevin Duling wrote:
> `ManagementAgent` has `System.setProperty(PULSE_EMBEDDED_PROP, "true");` at line within `if (agentUtil.isWebApplicationAvailable(gemfireWar, pulseWar, gemfireAPIWar)) {`
I believe we should set the system property for pulse jmx port here as well.
> On March 21, 2017, 5:43 p.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?
>
> Kevin Duling wrote:
> 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?
In the embeded case, I don't think that property file is even used....
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169587
-----------------------------------------------------------
On March 21, 2017, 4:35 p.m., Kevin Duling wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57796/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 4:35 p.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
>
>
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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
>
>
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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
>
>
Re: Review Request 57796: 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
Posted by Jinmei Liao <ji...@pivotal.io>.
> On March 21, 2017, 5:43 p.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?
>
> Kevin Duling wrote:
> 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?
>
> Jinmei Liao wrote:
> In the embeded case, I don't think that property file is even used....
>
> Kevin Duling wrote:
> Not quite correct. In the case of embedded, `pulse.host` and `pulse.port` are ignored, but the rest of the properties are loaded and used. I had tried to use `jmx-manager-port` before, but the properties are not set in the system. Instead, they are passed down as a properties object and visibility is lost once we reach `line 137` in `ManagementAgent.startAgent()`. At this point, we turn it over to Jetty to start the service, but the properties aren't handed off. I'm not sure they can be without setting a system property. This is why I chose to use the `pulse.port` parameter that Pulse normally looks for.
In the end, I would like to see user do not have to change a property file in order to get the imbeded pulse to work correctly when starting a locator on a non-default jmx port. The work flow should just be usual:
1) in gfsh, when user do: "start locator --jmx-manager-port=2000" (or via proeprty file)
2) fire up a browser, type in localhost:7070/pulse and pulse should just work.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169587
-----------------------------------------------------------
On March 21, 2017, 4:35 p.m., Kevin Duling wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57796/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 4:35 p.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
>
>
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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?
>
> Kevin Duling wrote:
> 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?
>
> Jinmei Liao wrote:
> In the embeded case, I don't think that property file is even used....
>
> Kevin Duling wrote:
> Not quite correct. In the case of embedded, `pulse.host` and `pulse.port` are ignored, but the rest of the properties are loaded and used. I had tried to use `jmx-manager-port` before, but the properties are not set in the system. Instead, they are passed down as a properties object and visibility is lost once we reach `line 137` in `ManagementAgent.startAgent()`. At this point, we turn it over to Jetty to start the service, but the properties aren't handed off. I'm not sure they can be without setting a system property. This is why I chose to use the `pulse.port` parameter that Pulse normally looks for.
>
> Jinmei Liao wrote:
> In the end, I would like to see user do not have to change a property file in order to get the imbeded pulse to work correctly when starting a locator on a non-default jmx port. The work flow should just be usual:
> 1) in gfsh, when user do: "start locator --jmx-manager-port=2000" (or via proeprty file)
> 2) fire up a browser, type in localhost:7070/pulse and pulse should just work.
Agreed, and tested that scenario. Works fine.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169587
-----------------------------------------------------------
On March 21, 2017, 2:48 p.m., Kevin Duling wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57796/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 2:48 p.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-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
> geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
> 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/2/
>
>
> Testing
> -------
>
> precheckin restarted
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57796: 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
Posted by Jinmei Liao <ji...@pivotal.io>.
> On March 21, 2017, 5:43 p.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.
how is this IsEmbededMode system property set? I would think if we are starting pulse in the managementAgent, we should set this system property.
> On March 21, 2017, 5:43 p.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.
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?
> On March 21, 2017, 5:43 p.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?
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.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169587
-----------------------------------------------------------
On March 21, 2017, 4:35 p.m., Kevin Duling wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57796/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 4:35 p.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
>
>
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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.
>
> Kevin Duling wrote:
> `ManagementAgent` has `System.setProperty(PULSE_EMBEDDED_PROP, "true");` at line within `if (agentUtil.isWebApplicationAvailable(gemfireWar, pulseWar, gemfireAPIWar)) {`
>
> Jinmei Liao wrote:
> I believe we should set the system property for pulse jmx port here as well.
If I set it as a system property, I can resolve this issue and the one below.
> 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?
>
> Kevin Duling wrote:
> 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?
>
> Jinmei Liao wrote:
> In the embeded case, I don't think that property file is even used....
Not quite correct. In the case of embedded, `pulse.host` and `pulse.port` are ignored, but the rest of the properties are loaded and used. I had tried to use `jmx-manager-port` before, but the properties are not set in the system. Instead, they are passed down as a properties object and visibility is lost once we reach `line 137` in `ManagementAgent.startAgent()`. At this point, we turn it over to Jetty to start the service, but the properties aren't handed off. I'm not sure they can be without setting a system property. This is why I chose to use the `pulse.port` parameter that Pulse normally looks for.
- 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
>
>
Re: Review Request 57796: 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
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169587
-----------------------------------------------------------
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java
Lines 72 (patched)
<https://reviews.apache.org/r/57796/#comment242009>
I would hope we shouldn't need to manully set this in the test.
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java
Lines 159 (patched)
<https://reviews.apache.org/r/57796/#comment242005>
Is this system proeprty set manually or by the locator/server when starting up the pulse service?
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java
Lines 160 (patched)
<https://reviews.apache.org/r/57796/#comment242006>
Which StringUtils is this? I believe we can use apache common's StringUtils.isBlank to do this.
geode-pulse/src/main/resources/pulse.properties
Line 28 (original)
<https://reviews.apache.org/r/57796/#comment242008>
I don't think we need to change this. This property file is only used when pulse is not in the embedded mode.
- Jinmei Liao
On March 21, 2017, 4:35 p.m., Kevin Duling wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57796/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 4:35 p.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
>
>
Re: Review Request 57796: 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
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169637
-----------------------------------------------------------
Fix it, then Ship it!
Fix it and Ship it!
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java
Lines 72 (patched)
<https://reviews.apache.org/r/57796/#comment242114>
Just in case we eventually change integrationTest to not forkEvery 1, we should add this in to undo the System.setProperty:
```java
@ClassRule
public static RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
```
Or code it by hand in @AfterClass.
- Kirk Lund
On March 21, 2017, 9:48 p.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:48 p.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-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
> geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
> 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/2/
>
>
> Testing
> -------
>
> precheckin restarted
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57796: 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
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169645
-----------------------------------------------------------
Ship it!
Ship It!
- Jinmei Liao
On March 21, 2017, 11:22 p.m., Kevin Duling wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57796/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 11:22 p.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-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
> geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
> 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/3/
>
>
> Testing
> -------
>
> precheckin restarted
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/
-----------------------------------------------------------
(Updated March 21, 2017, 4:22 p.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 (updated)
-----
geode-assembly/build.gradle 1900896da96afdcc7c776f0cd98a2aea6840fb1d
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java 57711258fbbc73570656e14ee8f05550ae32e891
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
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/3/
Changes: https://reviews.apache.org/r/57796/diff/2-3/
Testing
-------
precheckin restarted
Thanks,
Kevin Duling
Re: Review Request 57796: 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
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/#review169636
-----------------------------------------------------------
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java
Lines 72 (patched)
<https://reviews.apache.org/r/57796/#comment242112>
remove this and the test should still pass.
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
Lines 270 (patched)
<https://reviews.apache.org/r/57796/#comment242124>
declare a PUSE_PORT_PROP here to be the same value of the PulsePort.SYSTEM_PROPERTY_PULSE_PORT
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java
Lines 20 (patched)
<https://reviews.apache.org/r/57796/#comment242119>
We should not introduce this dependency here. Pulse needs to be run separately without the gemfire jars.
geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/PulseAppListener.java
Lines 152 (patched)
<https://reviews.apache.org/r/57796/#comment242123>
Should not use ConfigurationProperties here, Use PuseConstants.SYSTEM_PROPERTY_PULSE_PORT
- Jinmei Liao
On March 21, 2017, 9:48 p.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:48 p.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-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
> geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
> 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/2/
>
>
> Testing
> -------
>
> precheckin restarted
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/
-----------------------------------------------------------
(Updated March 21, 2017, 2:48 p.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-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
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/2/
Testing (updated)
-------
precheckin restarted
Thanks,
Kevin Duling
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57796/
-----------------------------------------------------------
(Updated March 21, 2017, 2:48 p.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 (updated)
-----
geode-assembly/build.gradle 1900896da96afdcc7c776f0cd98a2aea6840fb1d
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java 57711258fbbc73570656e14ee8f05550ae32e891
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java e88360ba1506f1a7b9c7df87899d5ec19abec630
geode-pulse/build.gradle 298ae5a8a32d621defd3336c21614c588b2ac7dc
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/2/
Changes: https://reviews.apache.org/r/57796/diff/1-2/
Testing
-------
precheckin successful
Thanks,
Kevin Duling
Re: Review Request 57796: 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
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
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 (updated)
-------
precheckin successful
Thanks,
Kevin Duling