You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2016/11/12 00:24:36 UTC
Review Request 53685: GEODE-1955: properly disconnect gfsh session so
that it won't leave heartbeat thread around to pollute other tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53685/
-----------------------------------------------------------
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
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53685/#review155926
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Duling
On Nov. 14, 2016, 12: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, 12: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
>
>
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53685/#review157416
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Nov. 28, 2016, 11:27 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53685/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 11:27 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/distributed/ConfigurationProperties.java c58a398ef9094ceecde2d7c6e806931dff872552
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java 3a2660cca88a0ae4070eab7c496e2079bfb63a7d
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java 3731d4d19ea7227d91e9714ccf2b56424098aaf3
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java 0fa47d434d599de81acebc275175eb087e9deb79
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 0c16c1a628a4785bba302ed651e3017e2ce7a563
>
> Diff: https://reviews.apache.org/r/53685/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53685/
-----------------------------------------------------------
(Updated Nov. 28, 2016, 11:27 p.m.)
Review request for geode, Kevin Duling, Kirk Lund, and Udo Kohlmeyer.
Changes
-------
Added the ignored tests back and fix the issues brought up in the review
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 (updated)
-----
geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java c58a398ef9094ceecde2d7c6e806931dff872552
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java 3a2660cca88a0ae4070eab7c496e2079bfb63a7d
geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java 3731d4d19ea7227d91e9714ccf2b56424098aaf3
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java 0fa47d434d599de81acebc275175eb087e9deb79
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 0c16c1a628a4785bba302ed651e3017e2ce7a563
Diff: https://reviews.apache.org/r/53685/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Jinmei Liao <ji...@pivotal.io>.
> 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
>
>
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Udo Kohlmeyer <uk...@gmail.com>.
> 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
>
>
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53685/#review156510
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java (lines 163 - 164)
<https://reviews.apache.org/r/53685/#comment226693>
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.
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java (line 243)
<https://reviews.apache.org/r/53685/#comment226694>
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-`
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (lines 101 - 112)
<https://reviews.apache.org/r/53685/#comment226697>
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.
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (lines 115 - 138)
<https://reviews.apache.org/r/53685/#comment226695>
I don't think you should ignore this test. Maybe mark them as flaky, but don't remove them.
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (line 128)
<https://reviews.apache.org/r/53685/#comment226696>
I don't think you should ignore this test. Maybe mark them as flaky, but don't remove them.
- Udo Kohlmeyer
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
>
>
Re: Review Request 53685: GEODE-1955: properly disconnect gfsh
session so
that it won't leave heartbeat thread around to pollute other tests
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
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 (updated)
-----
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