Re: Review Request 62132: GEODE-3277: Fix error path constructors of Launcher inner State classess

2017-09-08 Thread Jared Stewart 

---
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

2017-09-08 Thread Jinmei Liao 

---
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

2017-09-08 Thread Ken Howe 

---
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

2017-09-07 Thread Jinmei Liao 

---
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)


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)


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

2017-09-07 Thread Ken Howe 

---
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

Re: Review Request 62132: GEODE-3277: Fix error path constructors of Launcher inner State classess

2017-09-07 Thread Ken Howe 


> 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)
> > 
> >
> > 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

2017-09-07 Thread Jared Stewart 

---
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)


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
> 
>

Review Request 62132: GEODE-3277: Fix error path constructors of Launcher inner State classess

2017-09-06 Thread Ken Howe 

---
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