You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Ken Howe <kh...@pivotal.io> on 2017/09/06 20:10:06 UTC
Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/
-----------------------------------------------------------
Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
Repository: geode
Description
-------
Updated tests for changes in the error constructors for ServerState and
LocatorState.
Minor spelling corrections.
This reintroduces changes that were reverted due to merge conflicts with
the previous state of the develop branch
Diffs
-----
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
Diff: https://reviews.apache.org/r/62132/diff/1/
Testing
-------
Precheckin is green
Thanks,
Ken Howe
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Ken Howe <kh...@pivotal.io>.
> On Sept. 7, 2017, 6:16 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
> > Line 1099 (original), 1092 (patched)
> > <https://reviews.apache.org/r/62132/diff/1/?file=1816597#file1816597line1101>
> >
> > Do you think there is any value in logging the full stacktrace of the exception? It looks like that never gets logged anywhere.
I can take that out - I added quite a bit of extra logging while debugging that isn't normally necessary.
- Ken
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review184852
-----------------------------------------------------------
On Sept. 6, 2017, 8:10 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 8:10 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
>
> Minor spelling corrections.
>
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
> geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
>
>
> Diff: https://reviews.apache.org/r/62132/diff/1/
>
>
> Testing
> -------
>
> Precheckin is green
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review184852
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
Line 1099 (original), 1092 (patched)
<https://reviews.apache.org/r/62132/#comment261049>
Do you think there is any value in logging the full stacktrace of the exception? It looks like that never gets logged anywhere.
- Jared Stewart
On Sept. 6, 2017, 8:10 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 8:10 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
>
> Minor spelling corrections.
>
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
> geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
>
>
> Diff: https://reviews.apache.org/r/62132/diff/1/
>
>
> Testing
> -------
>
> Precheckin is green
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review185020
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On Sept. 8, 2017, 7:58 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 7:58 p.m.)
>
>
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
>
> Minor spelling corrections.
>
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
> geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
>
>
> Diff: https://reviews.apache.org/r/62132/diff/3/
>
>
> Testing
> -------
>
> Precheckin was green except for known problem with a new protobuf test and 1 apparently flaky test in HARQueueNewImplDUnitTest that passed on a second run.
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review185008
-----------------------------------------------------------
Ship it!
Ship It!
- Jinmei Liao
On Sept. 8, 2017, 7:58 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 7:58 p.m.)
>
>
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
>
> Minor spelling corrections.
>
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
> geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
>
>
> Diff: https://reviews.apache.org/r/62132/diff/3/
>
>
> Testing
> -------
>
> Precheckin was green except for known problem with a new protobuf test and 1 apparently flaky test in HARQueueNewImplDUnitTest that passed on a second run.
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/
-----------------------------------------------------------
(Updated Sept. 8, 2017, 7:58 p.m.)
Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
Changes
-------
Changes per review
Repository: geode
Description
-------
Updated tests for changes in the error constructors for ServerState and
LocatorState.
Minor spelling corrections.
This reintroduces changes that were reverted due to merge conflicts with
the previous state of the develop branch
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
Diff: https://reviews.apache.org/r/62132/diff/3/
Changes: https://reviews.apache.org/r/62132/diff/2-3/
Testing (updated)
-------
Precheckin was green except for known problem with a new protobuf test and 1 apparently flaky test in HARQueueNewImplDUnitTest that passed on a second run.
Thanks,
Ken Howe
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/#review184942
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
Line 2026 (original), 2019 (patched)
<https://reviews.apache.org/r/62132/#comment261147>
just noticed the discrepencies between the parameters passed to this constructor and the one above.
The one above calls launcher.getBindAddressAsString while this one ues getBindAddressAsSTring(launcher) which is private, probably we can do some code cleanup here?
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java
Lines 2607 (patched)
<https://reviews.apache.org/r/62132/#comment261148>
Is it possible to consolidate this method with the one in the LocatorState, seems very similar.
- Jinmei Liao
On Sept. 7, 2017, 10:29 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62132/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 10:29 p.m.)
>
>
> Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Updated tests for changes in the error constructors for ServerState and
> LocatorState.
>
> Minor spelling corrections.
>
> This reintroduces changes that were reverted due to merge conflicts with
> the previous state of the develop branch
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
> geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
> geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
> geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
> geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
>
>
> Diff: https://reviews.apache.org/r/62132/diff/2/
>
>
> Testing
> -------
>
> Precheckin is green
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 62132: GEODE-3277: Fix error path constructors of
Launcher inner State classess
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62132/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 10:29 p.m.)
Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg.
Changes
-------
Updated debug messages for both ```ServerLauncher``` and ```LocatorLanucher```
Repository: geode
Description
-------
Updated tests for changes in the error constructors for ServerState and
LocatorState.
Minor spelling corrections.
This reintroduces changes that were reverted due to merge conflicts with
the previous state of the develop branch
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 83c1ab533e3dea323a8a99f7002b9464a54dfc25
geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java ae64691605130c9b212a3a33bb65ae37b28af02b
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java 72ccfbbc83b18e8bc32759dbeabaf2f9ef4c2f45
geode-core/src/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java 9fce94e89a369094a2383eb9103f2f43a8ff3013
geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java 29ddaaf6692565a9afb8c528790b35798d118a31
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560
geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java 2bcd994d4d14888adfdf68abef5acbc068b6fea8
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java a9ce889006800523505dace6e0b4696c9911d205
Diff: https://reviews.apache.org/r/62132/diff/2/
Changes: https://reviews.apache.org/r/62132/diff/1-2/
Testing
-------
Precheckin is green
Thanks,
Ken Howe