Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-17 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/
---

(Updated May 18, 2017, midnight)


Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Swapnil Bawaskar.


Changes
---

Updated w.r.t. requested changes.  Optimized imports in all touched files.


Repository: geode


Description
---

*   geode.internal.lang.StringUtils has been deprecated.  In the interim, it 
has been heavily refactored and extends commons.lang.StringUtils.
*
*   Renamed:
*   --  EMPTY_STRING -> EMPTY (inherited)
*   --  toUpperCase  -> upperCase (inherited)
*   --  toLowerCase  -> lowerCase (inherited)
*   --  padEnding-> rightPad (inherited)
*
*   Removed:
*   --  EMPTY_STRING_ARRAY; usage replaced with 
commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
*   --  SPACES
*   --  UTF_8; rare usage replaced with raw string
*   --  concat; usage replaced with commons.lang.join, refactoring as necessary.
*   --  getLettersOnly
*   --  getSpaces
*   --  truncate
*   --  valueOf; usage refactored to use defaultString
*
*   Refactored
*   --  defaultIfBlank: previously relied on varargs and could return null.  
Usage refactored to allow inheritance from commons.
*   --  defaultString(s, EMPTY) refactored to use standard signature 
defaultString(s) for consistency throughout codebase.
*   --  isBlank: usage refactored to resolve discrepancies with 
commons.lang.isBlank, which is now inherited.
*   --  isEmpty: usage refactored to resolve discrepancies with 
commons.lang.isEmpty, which is now inherited.
*
*   Code Cleanup:
*   --  Many uses of !isBlank -> isNotBlank
*   --  Changes suggested by Inspections on most touched files.
*   -- Explicit  -> <> when type is inferable
*   -- while loops operating on iterators converted to for each loops
*   -- for loops operating on array indices converted to for each loops
*   --  Various string typos corrected.
*   --  isEmpty(s.trim()) -> isBlank(s)
*   --  s.trim().isEmpty() -> isEmpty(s)
*   --  Removed some instances of 'dead' code
*
*   Qualitative Changes:
*   --  The following functions now throw an error when called with a null 
string input:
*   --  *  LocatorLauncher.Builder.setMemberName
*   --  *  ServerLauncher.Builder.setMemberName
*   --  *  ServerLauncher.Builder.setHostnameForClients
*   --  (Unit tests added to capture these changes)
*
*   Notes:
*   --  StringUtils.wraps may be inherited from Apache Commons when the 
dependency is updated.
*   --  AbstractLauncher.getMember has the documented behavior of returning 
null when both MemberName and ID are blank.  Is this the best behavior for this 
method?

==

I'm going through the diff myself now and noticing a lot of refactorings that 
happen in dead code.  I'm going back to cull the dead code entirely, but wanted 
to go ahead and get this diff posted.


Diffs (updated)
-

  
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
 5277e57 
  
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
 0554f69 
  
geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPITestBase.java
 f2e90a4 
  
geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
 d531cc1 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
 f40ab3e 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
 4e52a67 
  geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java 
feba893 
  geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
641e009 
  geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
b2d0151 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterConfigurationService.java
 10623b4 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
 ad5e04d 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
 4bf010b 
  
geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java 
78b2944 
  
geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
 af3cb5d 
  
geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
 55e3542 
  geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java 
185fde7 
  
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/HandShake.java
 0b11bf1 
  geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java 
499d546 
  geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
2e47556

Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-17 Thread Jared Stewart 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/#review175280
---




geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
Line 224 (original), 224 (patched)


Looks like this comment may have been changed accidentally. Actually, since 
we're already looking around in this class, I think all of these "Not yet used" 
method comments should be deleted.



geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
Line 214 (original), 214 (patched)


Delete this comment



geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java
Line 504 (original), 507 (patched)


There is no need to pass the empty string into this constructor, `new 
StringBuilder();` will suffice.



geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java
Line 157 (original)


Did you intended to remove these assertions?



geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java
Line 194 (original)


Did you intended to remove these assertions?



geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java
Line 165 (original)


Did you intended to remove these assertions?



geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java
Line 280 (original)


Did you intended to remove these assertions?


- Jared Stewart


On May 16, 2017, 11:25 p.m., Patrick Rhomberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59323/
> ---
> 
> (Updated May 16, 2017, 11:25 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, 
> and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> *   Renamed:
> *   --  EMPTY_STRING -> EMPTY (inherited)
> *   --  toUpperCase  -> upperCase (inherited)
> *   --  toLowerCase  -> lowerCase (inherited)
> *   --  padEnding-> rightPad (inherited)
> *
> *   Removed:
> *   --  EMPTY_STRING_ARRAY; usage replaced with 
> commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
> *   --  SPACES
> *   --  UTF_8; rare usage replaced with raw string
> *   --  concat; usage replaced with commons.lang.join, refactoring as 
> necessary.
> *   --  getLettersOnly
> *   --  getSpaces
> *   --  truncate
> *   --  valueOf; usage refactored to use defaultString
> *
> *   Refactored
> *   --  defaultIfBlank: previously relied on varargs and could return null.  
> Usage refactored to allow inheritance from commons.
> *   --  defaultString(s, EMPTY) refactored to use standard signature 
> defaultString(s) for consistency throughout codebase.
> *   --  isBlank: usage refactored to resolve discrepancies with 
> commons.lang.isBlank, which is now inherited.
> *   --  isEmpty: usage refactored to resolve discrepancies with 
> commons.lang.isEmpty, which is now inherited.
> *
> *   Code Cleanup:
> *   --  Many uses of !isBlank -> isNotBlank
> *   --  Changes suggested by Inspections on most touched files.
> *   -- Explicit  -> <> when type is inferable
> *   -- while loops operating on iterators converted to for each loops
> *   -- for loops operating on array indices converted to for each loops
> *   --  Various string typos corrected.
> *   --  isEmpty(s.trim()) -> isBlank(s)
> *   --  s.trim().isEmpty() -> isEmpty(s)
> *   --  Removed some instances of 'dead' code
> *
> *   Qualitative Changes:
> *   --  The following functions now throw an error when called with a null 
> string input:
> *   --  *  LocatorLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setHostnameForClients
> *
> *   Notes:
> *   --  StringUtils.wraps may be inherited from Apache Commons when the 
> dependency is updated.
> *   --  AbstractLauncher.getMember has the documented behavior of returning 
> null when both MemberName and ID are blank.  Is this the best behavior for 
> this method?
> 
> ==
> 
> I'm going through the diff myself now and noticing a lot of refactorings that 
> happen in dead code.  I'm going back to cull the dead code entirely, but 
> wanted to go ahead and get this diff posted.
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
>  5277e57 
>   
>

Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-17 Thread Jinmei Liao 


> On May 17, 2017, 5:32 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java
> > Line 29 (original), 29 (patched)
> > 
> >
> > I would not make this class extends apache common's StringUtils. We 
> > should always encourage devleopers to use the apache's String utils, and 
> > only for very specific cases, we would want to use our own.

You can make this change after you commited this. I would imagine this would 
touch more files.


- Jinmei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/#review175278
---


On May 16, 2017, 11:25 p.m., Patrick Rhomberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59323/
> ---
> 
> (Updated May 16, 2017, 11:25 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, 
> and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> *   Renamed:
> *   --  EMPTY_STRING -> EMPTY (inherited)
> *   --  toUpperCase  -> upperCase (inherited)
> *   --  toLowerCase  -> lowerCase (inherited)
> *   --  padEnding-> rightPad (inherited)
> *
> *   Removed:
> *   --  EMPTY_STRING_ARRAY; usage replaced with 
> commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
> *   --  SPACES
> *   --  UTF_8; rare usage replaced with raw string
> *   --  concat; usage replaced with commons.lang.join, refactoring as 
> necessary.
> *   --  getLettersOnly
> *   --  getSpaces
> *   --  truncate
> *   --  valueOf; usage refactored to use defaultString
> *
> *   Refactored
> *   --  defaultIfBlank: previously relied on varargs and could return null.  
> Usage refactored to allow inheritance from commons.
> *   --  defaultString(s, EMPTY) refactored to use standard signature 
> defaultString(s) for consistency throughout codebase.
> *   --  isBlank: usage refactored to resolve discrepancies with 
> commons.lang.isBlank, which is now inherited.
> *   --  isEmpty: usage refactored to resolve discrepancies with 
> commons.lang.isEmpty, which is now inherited.
> *
> *   Code Cleanup:
> *   --  Many uses of !isBlank -> isNotBlank
> *   --  Changes suggested by Inspections on most touched files.
> *   -- Explicit  -> <> when type is inferable
> *   -- while loops operating on iterators converted to for each loops
> *   -- for loops operating on array indices converted to for each loops
> *   --  Various string typos corrected.
> *   --  isEmpty(s.trim()) -> isBlank(s)
> *   --  s.trim().isEmpty() -> isEmpty(s)
> *   --  Removed some instances of 'dead' code
> *
> *   Qualitative Changes:
> *   --  The following functions now throw an error when called with a null 
> string input:
> *   --  *  LocatorLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setHostnameForClients
> *
> *   Notes:
> *   --  StringUtils.wraps may be inherited from Apache Commons when the 
> dependency is updated.
> *   --  AbstractLauncher.getMember has the documented behavior of returning 
> null when both MemberName and ID are blank.  Is this the best behavior for 
> this method?
> 
> ==
> 
> I'm going through the diff myself now and noticing a lot of refactorings that 
> happen in dead code.  I'm going back to cull the dead code entirely, but 
> wanted to go ahead and get this diff posted.
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
>  5277e57 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
>  0554f69 
>   
> geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPITestBase.java
>  f2e90a4 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
>  d531cc1 
>   
> geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
>  f40ab3e 
>   
> geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
>  4e52a67 
>   geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java 
> feba893 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> 641e009 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> b2d0151 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
>  ad5e04d 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
>  4bf010b 
>   
>

Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-17 Thread Jinmei Liao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/#review175278
---




geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java
Line 29 (original), 29 (patched)


I would not make this class extends apache common's StringUtils. We should 
always encourage devleopers to use the apache's String utils, and only for very 
specific cases, we would want to use our own.


- Jinmei Liao


On May 16, 2017, 11:25 p.m., Patrick Rhomberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59323/
> ---
> 
> (Updated May 16, 2017, 11:25 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, 
> and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> *   Renamed:
> *   --  EMPTY_STRING -> EMPTY (inherited)
> *   --  toUpperCase  -> upperCase (inherited)
> *   --  toLowerCase  -> lowerCase (inherited)
> *   --  padEnding-> rightPad (inherited)
> *
> *   Removed:
> *   --  EMPTY_STRING_ARRAY; usage replaced with 
> commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
> *   --  SPACES
> *   --  UTF_8; rare usage replaced with raw string
> *   --  concat; usage replaced with commons.lang.join, refactoring as 
> necessary.
> *   --  getLettersOnly
> *   --  getSpaces
> *   --  truncate
> *   --  valueOf; usage refactored to use defaultString
> *
> *   Refactored
> *   --  defaultIfBlank: previously relied on varargs and could return null.  
> Usage refactored to allow inheritance from commons.
> *   --  defaultString(s, EMPTY) refactored to use standard signature 
> defaultString(s) for consistency throughout codebase.
> *   --  isBlank: usage refactored to resolve discrepancies with 
> commons.lang.isBlank, which is now inherited.
> *   --  isEmpty: usage refactored to resolve discrepancies with 
> commons.lang.isEmpty, which is now inherited.
> *
> *   Code Cleanup:
> *   --  Many uses of !isBlank -> isNotBlank
> *   --  Changes suggested by Inspections on most touched files.
> *   -- Explicit  -> <> when type is inferable
> *   -- while loops operating on iterators converted to for each loops
> *   -- for loops operating on array indices converted to for each loops
> *   --  Various string typos corrected.
> *   --  isEmpty(s.trim()) -> isBlank(s)
> *   --  s.trim().isEmpty() -> isEmpty(s)
> *   --  Removed some instances of 'dead' code
> *
> *   Qualitative Changes:
> *   --  The following functions now throw an error when called with a null 
> string input:
> *   --  *  LocatorLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setHostnameForClients
> *
> *   Notes:
> *   --  StringUtils.wraps may be inherited from Apache Commons when the 
> dependency is updated.
> *   --  AbstractLauncher.getMember has the documented behavior of returning 
> null when both MemberName and ID are blank.  Is this the best behavior for 
> this method?
> 
> ==
> 
> I'm going through the diff myself now and noticing a lot of refactorings that 
> happen in dead code.  I'm going back to cull the dead code entirely, but 
> wanted to go ahead and get this diff posted.
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
>  5277e57 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
>  0554f69 
>   
> geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPITestBase.java
>  f2e90a4 
>   
> geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
>  d531cc1 
>   
> geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
>  f40ab3e 
>   
> geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
>  4e52a67 
>   geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java 
> feba893 
>   geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
> 641e009 
>   geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
> b2d0151 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
>  ad5e04d 
>   
> geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
>  4bf010b 
>   
> geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java
>  78b2944 
>   
> geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
>  af3cb5d 
>   
>

Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-16 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/
---

(Updated May 16, 2017, 11:25 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Swapnil Bawaskar.


Changes
---

Reverted a few files that were touched during aggressive name refactoring.  
Purged dead code from internal StringUtils.


Repository: geode


Description
---

*   Renamed:
*   --  EMPTY_STRING -> EMPTY (inherited)
*   --  toUpperCase  -> upperCase (inherited)
*   --  toLowerCase  -> lowerCase (inherited)
*   --  padEnding-> rightPad (inherited)
*
*   Removed:
*   --  EMPTY_STRING_ARRAY; usage replaced with 
commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
*   --  SPACES
*   --  UTF_8; rare usage replaced with raw string
*   --  concat; usage replaced with commons.lang.join, refactoring as necessary.
*   --  getLettersOnly
*   --  getSpaces
*   --  truncate
*   --  valueOf; usage refactored to use defaultString
*
*   Refactored
*   --  defaultIfBlank: previously relied on varargs and could return null.  
Usage refactored to allow inheritance from commons.
*   --  defaultString(s, EMPTY) refactored to use standard signature 
defaultString(s) for consistency throughout codebase.
*   --  isBlank: usage refactored to resolve discrepancies with 
commons.lang.isBlank, which is now inherited.
*   --  isEmpty: usage refactored to resolve discrepancies with 
commons.lang.isEmpty, which is now inherited.
*
*   Code Cleanup:
*   --  Many uses of !isBlank -> isNotBlank
*   --  Changes suggested by Inspections on most touched files.
*   -- Explicit  -> <> when type is inferable
*   -- while loops operating on iterators converted to for each loops
*   -- for loops operating on array indices converted to for each loops
*   --  Various string typos corrected.
*   --  isEmpty(s.trim()) -> isBlank(s)
*   --  s.trim().isEmpty() -> isEmpty(s)
*   --  Removed some instances of 'dead' code
*
*   Qualitative Changes:
*   --  The following functions now throw an error when called with a null 
string input:
*   --  *  LocatorLauncher.Builder.setMemberName
*   --  *  ServerLauncher.Builder.setMemberName
*   --  *  ServerLauncher.Builder.setHostnameForClients
*
*   Notes:
*   --  StringUtils.wraps may be inherited from Apache Commons when the 
dependency is updated.
*   --  AbstractLauncher.getMember has the documented behavior of returning 
null when both MemberName and ID are blank.  Is this the best behavior for this 
method?

==

I'm going through the diff myself now and noticing a lot of refactorings that 
happen in dead code.  I'm going back to cull the dead code entirely, but wanted 
to go ahead and get this diff posted.


Diffs (updated)
-

  
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
 5277e57 
  
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
 0554f69 
  
geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPITestBase.java
 f2e90a4 
  
geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
 d531cc1 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
 f40ab3e 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
 4e52a67 
  geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java 
feba893 
  geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
641e009 
  geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
b2d0151 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
 ad5e04d 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
 4bf010b 
  
geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java 
78b2944 
  
geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java
 af3cb5d 
  
geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java
 55e3542 
  geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java 
499d546 
  geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
3485ede 
  
geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java
 629740c 
  geode-core/src/main/java/org/apache/geode/internal/process/signal/Signal.java 
faab4ed 
  
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java
 8366930 
  geode-core/src/main/java/org/apache/geode/internal/util/ArrayUtils.java 
6f1c7cc 
  geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java c1a1952

Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-16 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/#review175167
---



There are a few files that I've cleaned up a little, but it appears that any 
changes I made w.r.t. StringUtils were reverted.  Should I keep those cleanups 
in place, or should I prefer to touch fewer files?


geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java
Lines 1514-1518 (original), 1514-1518 (patched)


Remove dead code.



geode-core/src/main/java/org/apache/geode/cache/query/internal/CumulativeNonDistinctResults.java
Lines 198-200 (original), 198-201 (patched)


Remove dead code.



geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
Lines 214-216 (original), 214-217 (patched)


Remove dead code.



geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
Line 325 (original), 303-309 (patched)


hostnameForClients can potentially be null.
A null memberName gets reassigned and so that block can be removed.



geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
Line 325 (original), 303-309 (patched)


hostnameForClients can potentially be null, which would raise an error in 
the set.

A null memberName gets reassigned and so that block can be removed.


- Patrick Rhomberg


On May 16, 2017, 10:29 p.m., Patrick Rhomberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59323/
> ---
> 
> (Updated May 16, 2017, 10:29 p.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, 
> and Swapnil Bawaskar.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> *   Renamed:
> *   --  EMPTY_STRING -> EMPTY (inherited)
> *   --  toUpperCase  -> upperCase (inherited)
> *   --  toLowerCase  -> lowerCase (inherited)
> *   --  padEnding-> rightPad (inherited)
> *
> *   Removed:
> *   --  EMPTY_STRING_ARRAY; usage replaced with 
> commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
> *   --  SPACES
> *   --  UTF_8; rare usage replaced with raw string
> *   --  concat; usage replaced with commons.lang.join, refactoring as 
> necessary.
> *   --  getLettersOnly
> *   --  getSpaces
> *   --  truncate
> *   --  valueOf; usage refactored to use defaultString
> *
> *   Refactored
> *   --  defaultIfBlank: previously relied on varargs and could return null.  
> Usage refactored to allow inheritance from commons.
> *   --  defaultString(s, EMPTY) refactored to use standard signature 
> defaultString(s) for consistency throughout codebase.
> *   --  isBlank: usage refactored to resolve discrepancies with 
> commons.lang.isBlank, which is now inherited.
> *   --  isEmpty: usage refactored to resolve discrepancies with 
> commons.lang.isEmpty, which is now inherited.
> *
> *   Code Cleanup:
> *   --  Many uses of !isBlank -> isNotBlank
> *   --  Changes suggested by Inspections on most touched files.
> *   -- Explicit  -> <> when type is inferable
> *   -- while loops operating on iterators converted to for each loops
> *   -- for loops operating on array indices converted to for each loops
> *   --  Various string typos corrected.
> *   --  isEmpty(s.trim()) -> isBlank(s)
> *   --  s.trim().isEmpty() -> isEmpty(s)
> *   --  Removed some instances of 'dead' code
> *
> *   Qualitative Changes:
> *   --  The following functions now throw an error when called with a null 
> string input:
> *   --  *  LocatorLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setMemberName
> *   --  *  ServerLauncher.Builder.setHostnameForClients
> *
> *   Notes:
> *   --  StringUtils.wraps may be inherited from Apache Commons when the 
> dependency is updated.
> *   --  AbstractLauncher.getMember has the documented behavior of returning 
> null when both MemberName and ID are blank.  Is this the best behavior for 
> this method?
> 
> ==
> 
> I'm going through the diff myself now and noticing a lot of refactorings that 
> happen in dead code.  I'm going back to cull the dead code entirely, but 
> wanted to go ahead and get this diff posted.
> 
> 
> Diffs
> -
> 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
>  5277e57ae4d5d3901fddb627edfb11e4145a13f8 
>   
> geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
>

Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.

2017-05-16 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59323/
---

(Updated May 16, 2017, 10:29 p.m.)


Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, and 
Swapnil Bawaskar.


Summary (updated)
-

GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use 
commons.lang.StringUtils.


Repository: geode


Description
---

*   Renamed:
*   --  EMPTY_STRING -> EMPTY (inherited)
*   --  toUpperCase  -> upperCase (inherited)
*   --  toLowerCase  -> lowerCase (inherited)
*   --  padEnding-> rightPad (inherited)
*
*   Removed:
*   --  EMPTY_STRING_ARRAY; usage replaced with 
commons.lang.ArrayUtils.EMPTY_STRING_ARRAY
*   --  SPACES
*   --  UTF_8; rare usage replaced with raw string
*   --  concat; usage replaced with commons.lang.join, refactoring as necessary.
*   --  getLettersOnly
*   --  getSpaces
*   --  truncate
*   --  valueOf; usage refactored to use defaultString
*
*   Refactored
*   --  defaultIfBlank: previously relied on varargs and could return null.  
Usage refactored to allow inheritance from commons.
*   --  defaultString(s, EMPTY) refactored to use standard signature 
defaultString(s) for consistency throughout codebase.
*   --  isBlank: usage refactored to resolve discrepancies with 
commons.lang.isBlank, which is now inherited.
*   --  isEmpty: usage refactored to resolve discrepancies with 
commons.lang.isEmpty, which is now inherited.
*
*   Code Cleanup:
*   --  Many uses of !isBlank -> isNotBlank
*   --  Changes suggested by Inspections on most touched files.
*   -- Explicit  -> <> when type is inferable
*   -- while loops operating on iterators converted to for each loops
*   -- for loops operating on array indices converted to for each loops
*   --  Various string typos corrected.
*   --  isEmpty(s.trim()) -> isBlank(s)
*   --  s.trim().isEmpty() -> isEmpty(s)
*   --  Removed some instances of 'dead' code
*
*   Qualitative Changes:
*   --  The following functions now throw an error when called with a null 
string input:
*   --  *  LocatorLauncher.Builder.setMemberName
*   --  *  ServerLauncher.Builder.setMemberName
*   --  *  ServerLauncher.Builder.setHostnameForClients
*
*   Notes:
*   --  StringUtils.wraps may be inherited from Apache Commons when the 
dependency is updated.
*   --  AbstractLauncher.getMember has the documented behavior of returning 
null when both MemberName and ID are blank.  Is this the best behavior for this 
method?

==

I'm going through the diff myself now and noticing a lot of refactorings that 
happen in dead code.  I'm going back to cull the dead code entirely, but wanted 
to go ahead and get this diff posted.


Diffs
-

  
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsDUnitTest.java
 5277e57ae4d5d3901fddb627edfb11e4145a13f8 
  
geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommandsTest.java
 0554f69adbea10f30b5dac1580e7d87ec786d228 
  
geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPITestBase.java
 f2e90a44a3d08070aa1869df543f5b647cb18b22 
  geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java 
69f108714e42a9bf970a10f157491e36fa2b5dd3 
  
geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
 d531cc15529a9389cd1ebbcffd769e9d0b6f3e94 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/AttributeDescriptor.java
 f40ab3e5ecffc0bbd3c0f92e84a512cc5b34f496 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/CumulativeNonDistinctResults.java
 2565dd3db4f27df205c4f141d728c7ee441850d3 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/NWayMergeResults.java
 4e52a670924b437d464d23f45bf8541c86d847e9 
  geode-core/src/main/java/org/apache/geode/cache/query/internal/QRegion.java 
5f0d6e580576926283aa47b0005a34c15c03a20b 
  
geode-core/src/main/java/org/apache/geode/cache/query/internal/types/TypeUtils.java
 6c2399a63ec365abdc4513f6cf44a626d14963ad 
  geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java 
feba8937b216d374acd8357775d5d1806568b355 
  geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java 
641e009d5e9fe646684fe65d91110805fe589b91 
  geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 
b2d01511e4868b7055521901d378aa5efaefe203 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java
 ad5e04d4f11085cb3477da6a13aec0167761ed35 
  
geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/membership/GMSJoinLeave.java
 4bf010b42fc6d4d20392bba883cb802846ee1bcf 
  
geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java