geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jared Stewart <jstew...@pivotal.io>
Subject Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils.
Date Wed, 17 May 2017 18:02:07 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/59323/#comment248725>

    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)
<https://reviews.apache.org/r/59323/#comment248726>

    Delete this comment



geode-core/src/main/java/org/apache/geode/distributed/AbstractLauncher.java
Line 504 (original), 507 (patched)
<https://reviews.apache.org/r/59323/#comment248728>

    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)
<https://reviews.apache.org/r/59323/#comment248745>

    Did you intended to remove these assertions?



geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java
Line 194 (original)
<https://reviews.apache.org/r/59323/#comment248746>

    Did you intended to remove these assertions?



geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java
Line 165 (original)
<https://reviews.apache.org/r/59323/#comment248747>

    Did you intended to remove these assertions?



geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java
Line 280 (original)
<https://reviews.apache.org/r/59323/#comment248748>

    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 <T> -> <> 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 
>   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 
>   geode-core/src/main/java/org/apache/geode/management/internal/AgentUtil.java ba15508

>   geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 7c26297

>   geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
62310e8 
>   geode-core/src/main/java/org/apache/geode/management/internal/RestAgent.java 837e815

>   geode-core/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemBridge.java
ef643ac 
>   geode-core/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemMBean.java
a87b366 
>   geode-core/src/main/java/org/apache/geode/management/internal/beans/ManagementAdapter.java
7dce602 
>   geode-core/src/main/java/org/apache/geode/management/internal/beans/QueryDataFunction.java
f701d29 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java
5dfc1b8 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java
5b0651e 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/IndexCommands.java
9110a1a 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
101bae4 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java
c9e79b0 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java
59851da 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/IndexDetails.java
64bb512 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java
4383044 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java
9dbf59c 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java
9bd2bf9 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TableBuilder.java
da3c4ae 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java
7c80e0d 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java
e9d183e 
>   geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java
4410fea 
>   geode-core/src/main/java/org/apache/geode/management/internal/configuration/domain/XmlEntity.java
be74e84 
>   geode-core/src/main/java/org/apache/geode/management/internal/configuration/messages/ConfigurationRequest.java
837d99e 
>   geode-core/src/main/java/org/apache/geode/management/internal/configuration/messages/ConfigurationResponse.java
3248c98 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/AbstractCommandsController.java
0b64a44 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ConfigCommandsController.java
f468c65 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java
3a8ed82 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DeployCommandsController.java
fe5be62 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DiskStoreCommandsController.java
661340c 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DurableClientCommandsController.java
1d718b4 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
604bdee 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/FunctionCommandsController.java
77cfb40 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MiscellaneousCommandsController.java
d19aee1 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/PdxCommandsController.java
c68ee35 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/QueueCommandsController.java
396726e 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/RegionCommandsController.java
e503e56 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java
0ecb77f 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/WanCommandsController.java
fa5aa57 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/domain/Link.java
cef8cab 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/domain/LinkIndex.java
7d5bb37 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/http/HttpHeader.java
ee7e932 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvoker.java
eeedf40 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/util/UriUtils.java
b83064e 
>   geode-core/src/test/java/org/apache/geode/cache/client/internal/LocatorTestBase.java
c3b349a 
>   geode-core/src/test/java/org/apache/geode/distributed/AbstractLauncherTest.java 62d4bdd

>   geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherIntegrationTest.java
5b53c6a 
>   geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java 06d6054

>   geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherIntegrationTest.java
7bd7462 
>   geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java f5d6271

>   geode-core/src/test/java/org/apache/geode/internal/cache/execute/Bug51193DUnitTest.java
0dfbe6c 
>   geode-core/src/test/java/org/apache/geode/internal/lang/ClassUtilsJUnitTest.java 3ec3b06

>   geode-core/src/test/java/org/apache/geode/internal/lang/StringUtilsJUnitTest.java f3e0abe

>   geode-core/src/test/java/org/apache/geode/internal/util/CollectionUtilsJUnitTest.java
36be8b8 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupportJUnitTest.java
91a59f8 
>   geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListIndexCommandDUnitTest.java
8bf1c43 
>   geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfig.java
fc920c4 
>   geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java
b2be12a 
>   geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsJUnitTest.java
d4dca4a 
>   geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/PropMockDataUpdater.java
6ee5b53 
>   geode-spark-connector/geode-functions/src/main/java/org/apache/geode/spark/connector/internal/geodefunctions/RetrieveRegionFunction.java
096e4d5 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/AbstractWebTestCase.java
eac0b8d 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java
37ec508 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvokerJUnitTest.java
c69013b 
> 
> 
> Diff: https://reviews.apache.org/r/59323/diff/2/
> 
> 
> Testing
> -------
> 
> Precheckin running.  Tests and LegacyTests already returned green.
> 
> 
> Thanks,
> 
> Patrick Rhomberg
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message