Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 3E77D200C86 for ; Wed, 17 May 2017 01:23:53 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 3D1F3160BC9; Tue, 16 May 2017 23:23:53 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 0D6B2160BC1 for ; Wed, 17 May 2017 01:23:51 +0200 (CEST) Received: (qmail 19194 invoked by uid 500); 16 May 2017 23:23:51 -0000 Mailing-List: contact dev-help@geode.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@geode.apache.org Delivered-To: mailing list dev@geode.apache.org Received: (qmail 19177 invoked by uid 99); 16 May 2017 23:23:50 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 16 May 2017 23:23:50 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id 678C4C1231; Tue, 16 May 2017 23:23:50 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 4.25 X-Spam-Level: **** X-Spam-Status: No, score=4.25 tagged_above=-999 required=6.31 tests=[HEADER_FROM_DIFFERENT_DOMAINS=0.001, HTML_MESSAGE=2, KAM_LAZY_DOMAIN_SECURITY=1, KAM_LOTSOFHASH=0.25, MANY_SPAN_IN_TEXT=1, RP_MATCHES_RCVD=-0.001] autolearn=disabled Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id ppBCRT3ckBiZ; Tue, 16 May 2017 23:23:43 +0000 (UTC) Received: from mailrelay1-us-west.apache.org (mailrelay1-us-west.apache.org [209.188.14.139]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTP id 46E7E5F666; Tue, 16 May 2017 23:23:43 +0000 (UTC) Received: from reviews.apache.org (unknown [10.41.0.12]) by mailrelay1-us-west.apache.org (ASF Mail Server at mailrelay1-us-west.apache.org) with ESMTP id 9BA12E002B; Tue, 16 May 2017 23:23:42 +0000 (UTC) Received: from reviews-vm2.apache.org (localhost [IPv6:::1]) by reviews.apache.org (ASF Mail Server at reviews-vm2.apache.org) with ESMTP id 632E7C40155; Tue, 16 May 2017 23:23:42 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3920882167491159423==" MIME-Version: 1.0 Subject: Re: Review Request 59323: GEODE-1994: Overhaul of internal.lang.StringUtils to extend and heavily use commons.lang.StringUtils. From: Patrick Rhomberg To: Jinmei Liao , Swapnil Bawaskar , Kirk Lund , Jared Stewart , Ken Howe Cc: Patrick Rhomberg , geode Date: Tue, 16 May 2017 23:23:42 -0000 Message-ID: <20170516232342.61683.16013@reviews-vm2.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Patrick Rhomberg X-ReviewGroup: geode X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/59323/ X-Sender: Patrick Rhomberg References: <20170516222906.61683.44849@reviews-vm2.apache.org> In-Reply-To: <20170516222906.61683.44849@reviews-vm2.apache.org> Reply-To: Patrick Rhomberg X-ReviewRequest-Repository: geode archived-at: Tue, 16 May 2017 23:23:53 -0000 --===============3920882167491159423== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 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 78b294475a49e568b267f69c4c6bdecd4338f4cb > geode-core/src/main/java/org/apache/geode/internal/ObjIdConcurrentMap.java 17894ad51428112c4c635b307a25047a8420621a > geode-core/src/main/java/org/apache/geode/internal/admin/remote/RemoteTransportConfig.java af3cb5d4f60b68ca19fe6dfdf52d0cdcbcc2c44d > geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java 55e354262e35f69e6e2182b5c28b573cf8fa282d > geode-core/src/main/java/org/apache/geode/internal/lang/StringUtils.java 499d546c828ce8ffa1d56f7d3cba8325d3486702 > geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 3485ede5955d2a3acad9b5c54ee5003596bd07f3 > geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java 629740c4fafa89c2a332d5a27293d42b7834ee34 > geode-core/src/main/java/org/apache/geode/internal/process/signal/Signal.java faab4edbfca3461fa6a6182ea3f44577743e8dd4 > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 8366930a3bf6a244d077c745fc810d77c4699c38 > geode-core/src/main/java/org/apache/geode/internal/util/ArrayUtils.java 6f1c7cc80b22f3e30c6900df6b28c86ef0421fd5 > geode-core/src/main/java/org/apache/geode/internal/util/IOUtils.java c1a1952621c00476163fc8aef397830889f1225d > geode-core/src/main/java/org/apache/geode/management/internal/AgentUtil.java ba155086183696bedcbb2feaa1546ba4a245a66d > geode-core/src/main/java/org/apache/geode/management/internal/JettyHelper.java 7c262973907a1f1ce8650a332704534653b48856 > geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java 62310e88aa9158b22b7d3fd977461813acbe038a > geode-core/src/main/java/org/apache/geode/management/internal/RestAgent.java 837e81555e9ba1588d3546dfd4ec4343e813ac55 > geode-core/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemBridge.java ef643ac02a7d422060c56fd5b1d0b76724334d79 > geode-core/src/main/java/org/apache/geode/management/internal/beans/DistributedSystemMBean.java a87b3668cde7688df5fe818477450ded0d56458c > geode-core/src/main/java/org/apache/geode/management/internal/beans/ManagementAdapter.java 7dce60213ea12ce6a6e36e926ef9a4a00d32bac3 > geode-core/src/main/java/org/apache/geode/management/internal/beans/QueryDataFunction.java f701d2979b175e8a02a241cf3a4761d6c1b4d096 > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 5dfc1b83b56884bd5a265280d5a6116fc2dbddff > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java 5b0651e91650c9ae56e0094036891ac6c684a477 > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/IndexCommands.java 9110a1a3c501127cf2d0a11d2b5d8474a7167990 > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 101bae4fb7599cdb01e34593d494d3e205502ae8 > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java c9e79b0c258f02eedbb05d19c8747a409b7ef323 > geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java 59851daf45cabfec107c56bb1327a27557c19fe2 > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java 423d781248458a450c2e98cd8a0e609c8656edb0 > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/IndexDetails.java 64bb5128372f90e8a85d5556847303d8b25e49b1 > geode-core/src/main/java/org/apache/geode/management/internal/cli/help/HelpBlock.java 4383044959de629b9caa8f26527b00f6e2c2b0b3 > geode-core/src/main/java/org/apache/geode/management/internal/cli/help/Helper.java 9dbf59c140394ce74fe950d57ec247206ce7c765 > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java 9bd2bf98c24e0a21e530f0dfbe6726163c689592 > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TableBuilder.java da3c4aea56ca0134d4c9f3824ca4ab3f617c21cf > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java 7c80e0d270eea429adf04972a9c07d6f75d2dcde > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/JmxOperationInvoker.java e9d183e4731d3d50935f57653c5054b8403cfdbc > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java 4410fea0be74d438d107f9def74e34a5fc838903 > geode-core/src/main/java/org/apache/geode/management/internal/configuration/domain/XmlEntity.java be74e84289c218b533443931db270c22ba4f06e1 > geode-core/src/main/java/org/apache/geode/management/internal/configuration/messages/ConfigurationRequest.java 837d99e52b914f5fbd01927a68ff6ce39517babf > geode-core/src/main/java/org/apache/geode/management/internal/configuration/messages/ConfigurationResponse.java 3248c98b05f7e47ffb331cab3418221a3af33e6b > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/AbstractCommandsController.java 0b64a4477bb00821798f91f15d43848ee5587ccb > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ConfigCommandsController.java f468c65c072f9684e2b16c8b84832273df9421ce > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java 3a8ed82eab68204dab75b557fa1aaf32ed409e04 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DeployCommandsController.java fe5be621a020fd9dde303e45deeb09da12f3972b > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DiskStoreCommandsController.java 661340c91d95f319be777ebb58659bbc7dc7c668 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DurableClientCommandsController.java 1d718b4c986c96fec2c261820f17e5765532d1a7 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java 604bdee9f30bea1fff42007cf87dbe5f2f2e26b0 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/FunctionCommandsController.java 77cfb402a309a43108b752674755b1bc6f24be61 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/MiscellaneousCommandsController.java d19aee13e8c88cbf139a1b344e3e01df242a2a06 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/PdxCommandsController.java c68ee3592c86931abb5197e633da0b4e02518483 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/QueueCommandsController.java 396726e0b7a7e54030309db2f0c0c6b9e550cefe > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/RegionCommandsController.java e503e561d951c3fb58259009e22f2e164a9c5630 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ShellCommandsController.java 0ecb77f9c413aaff1e945011f4e16cf6efb13fc2 > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/WanCommandsController.java fa5aa57a829118b49b76d2a791822aa03dcaf1de > geode-core/src/main/java/org/apache/geode/management/internal/web/domain/Link.java cef8cabf109e958937a17b46e8d9935980de9511 > geode-core/src/main/java/org/apache/geode/management/internal/web/domain/LinkIndex.java 7d5bb379d1df719fea672e2bec10197121f60e62 > geode-core/src/main/java/org/apache/geode/management/internal/web/http/HttpHeader.java ee7e932bcbc09952700cf423cd6a9287d46f7835 > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvoker.java eeedf4033e832b2422a81ce42a2f48b3575fda51 > geode-core/src/main/java/org/apache/geode/management/internal/web/util/UriUtils.java b83064e8d146028166b51159266813f1b87d1a96 > geode-core/src/test/java/org/apache/geode/cache/client/internal/LocatorTestBase.java c3b349a5f889b393552be7196b501e2cc5da5c66 > geode-core/src/test/java/org/apache/geode/cache30/MultiVMRegionTestCase.java ff996a279958131ba612d2b76d3e53a165916d9d > geode-core/src/test/java/org/apache/geode/distributed/AbstractLauncherTest.java 62d4bdd443d02aef910074e8b20d3963d7d1d301 > geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherIntegrationTest.java 5b53c6a33f2a269880232f726642ad2bebcc5976 > geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java 06d6054f524a4fd31fc8b8189b24aa3def10babd > geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherIntegrationTest.java 7bd7462e1e8394f5f8235d7a9c7fa6954735d6a7 > geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherTest.java f5d6271c3a3d598973b28c7423f609961dbc8272 > geode-core/src/test/java/org/apache/geode/internal/cache/ClientServerTransactionDUnitTest.java ec4e179709efe21bec09a0c8fa2bfc8153915ea8 > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionAPIDUnitTest.java 0b18ebce48fa4c6fa3ce648cfd6040cfaecf8206 > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionSingleNodeOperationsJUnitTest.java e69aa956f6a02f7a47d3c2622d937d13e98154cc > geode-core/src/test/java/org/apache/geode/internal/cache/execute/Bug51193DUnitTest.java 0dfbe6cc1580d890104d907e8d9efb388b892c24 > geode-core/src/test/java/org/apache/geode/internal/lang/ClassUtilsJUnitTest.java 3ec3b067bfc978268a16c68fb497e418daab0b1a > geode-core/src/test/java/org/apache/geode/internal/lang/StringUtilsJUnitTest.java f3e0abed3241b95cfd473649e5b3d94d908104a7 > geode-core/src/test/java/org/apache/geode/internal/util/CollectionUtilsJUnitTest.java 36be8b8e99549ea250491edbe9242afa075b7798 > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupportJUnitTest.java 91a59f87947baac724a2a9863d16ab16bdf8aa2b > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListIndexCommandDUnitTest.java 8bf1c4318190f42d83ef9c4c9ee1ff6fdfecdea0 > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfig.java fc920c43d3dbe88f93227194eb23e69a513c3d05 > geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java b2be12afb53df95acd6832032922efaa769e672c > geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommandsJUnitTest.java d4dca4adefe2070cca3d91abbb36723c615bbd34 > geode-pulse/src/test/java/org/apache/geode/tools/pulse/testbed/PropMockDataUpdater.java 6ee5b53744f31df90ff442e0997efeff3634c032 > geode-spark-connector/geode-functions/src/main/java/org/apache/geode/spark/connector/internal/geodefunctions/RetrieveRegionFunction.java 096e4d5cb71c1043c7c0754a02252b05b52ba9f3 > geode-web/src/test/java/org/apache/geode/management/internal/web/AbstractWebTestCase.java eac0b8d4f577bf5e80e75ed583fdd01297540e14 > geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java 37ec508b12280939f5be3f0310c47481b5e81195 > geode-web/src/test/java/org/apache/geode/management/internal/web/shell/RestHttpOperationInvokerJUnitTest.java c69013badbdee87a06a01938ba5be2918c1961b4 > > > Diff: https://reviews.apache.org/r/59323/diff/1/ > > > Testing > ------- > > Precheckin running. Tests and LegacyTests already returned green. > > > Thanks, > > Patrick Rhomberg > > --===============3920882167491159423==--