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/09/01 16:30:23 UTC
Review Request 51577: GEODE-1834: initilize the socketcreator
correctly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51577/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt and Kirk Lund.
Repository: geode
Description
-------
GEODE-1834: initilize the socketcreator correctly
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java fded3c3b1dc3cf6d2af31291c97adb9f35cd1150
geode-core/src/main/java/com/gemstone/gemfire/management/internal/JmxManagerLocatorRequest.java 861f51d4e0077440ab3128e6053c389cecfd9bb8
geode-core/src/test/java/com/gemstone/gemfire/internal/JSSESocketJUnitTest.java d0d906a8f86ce6a882beb79f91906f5ba5e0ec28
Diff: https://reviews.apache.org/r/51577/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 51577: GEODE-1834: initilize the socketcreator
correctly
Posted by Kirk Lund <ki...@gmail.com>.
> On Sept. 1, 2016, 5:26 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java, line 331
> > <https://reviews.apache.org/r/51577/diff/1/?file=1490166#file1490166line331>
> >
> > No calls ever pass in "true" for reinitialize. I think this shouldn't be added unless there's a code path that needs it. Also, we would need a new test added that exercises this arg as well.
>
> Jinmei Liao wrote:
> The original calls pass in "true", only the JmxManagerLocatorRequest will pass in "false"
Oops! I see that now on line 293 of SocketCreator. I closed the issue.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51577/#review147579
-----------------------------------------------------------
On Sept. 1, 2016, 4:30 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51577/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2016, 4:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1834: initilize the socketcreator correctly
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java fded3c3b1dc3cf6d2af31291c97adb9f35cd1150
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/JmxManagerLocatorRequest.java 861f51d4e0077440ab3128e6053c389cecfd9bb8
> geode-core/src/test/java/com/gemstone/gemfire/internal/JSSESocketJUnitTest.java d0d906a8f86ce6a882beb79f91906f5ba5e0ec28
>
> Diff: https://reviews.apache.org/r/51577/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 51577: GEODE-1834: initilize the socketcreator
correctly
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Sept. 1, 2016, 5:26 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java, line 331
> > <https://reviews.apache.org/r/51577/diff/1/?file=1490166#file1490166line331>
> >
> > No calls ever pass in "true" for reinitialize. I think this shouldn't be added unless there's a code path that needs it. Also, we would need a new test added that exercises this arg as well.
The original calls pass in "true", only the JmxManagerLocatorRequest will pass in "false"
> On Sept. 1, 2016, 5:26 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/management/internal/JmxManagerLocatorRequest.java, line 92
> > <https://reviews.apache.org/r/51577/diff/1/?file=1490167#file1490167line92>
> >
> > Since this class has no tests but we're trying to switch to TDD, you really should introduce a new JmxManagerLocatorRequestTest (UnitTest category) that exercises the modified method at a minimum.
Will add in a test
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51577/#review147579
-----------------------------------------------------------
On Sept. 1, 2016, 4:30 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51577/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2016, 4:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1834: initilize the socketcreator correctly
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java fded3c3b1dc3cf6d2af31291c97adb9f35cd1150
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/JmxManagerLocatorRequest.java 861f51d4e0077440ab3128e6053c389cecfd9bb8
> geode-core/src/test/java/com/gemstone/gemfire/internal/JSSESocketJUnitTest.java d0d906a8f86ce6a882beb79f91906f5ba5e0ec28
>
> Diff: https://reviews.apache.org/r/51577/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 51577: GEODE-1834: initilize the socketcreator
correctly
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51577/#review147579
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java (line 303)
<https://reviews.apache.org/r/51577/#comment214771>
No calls ever pass in "true" for reinitialize. I think this shouldn't be added unless there's a code path that needs it. Also, we would need a new test added that exercises this arg as well.
geode-core/src/main/java/com/gemstone/gemfire/management/internal/JmxManagerLocatorRequest.java (line 89)
<https://reviews.apache.org/r/51577/#comment214774>
Since this class has no tests but we're trying to switch to TDD, you really should introduce a new JmxManagerLocatorRequestTest (UnitTest category) that exercises the modified method at a minimum.
- Kirk Lund
On Sept. 1, 2016, 4:30 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51577/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2016, 4:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1834: initilize the socketcreator correctly
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/SocketCreator.java fded3c3b1dc3cf6d2af31291c97adb9f35cd1150
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/JmxManagerLocatorRequest.java 861f51d4e0077440ab3128e6053c389cecfd9bb8
> geode-core/src/test/java/com/gemstone/gemfire/internal/JSSESocketJUnitTest.java d0d906a8f86ce6a882beb79f91906f5ba5e0ec28
>
> Diff: https://reviews.apache.org/r/51577/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>