Re: Bug Numbers and Trac Numbers in comments
+1 - What Jake said. > On Feb 19, 2019, at 5:21 PM, Jacob Barrett wrote: > > Comments that don’t provide meaningful context beyond what is already > expressed in the code should be removed. A number to a system that the > general public can’t access is not meaningful. Delete or replace with > meaningful comment. > > -jake > > >> On Feb 19, 2019, at 1:41 PM, Michael Oleske wrote: >> >> Hey Geode Dev Friends! >> >> I was reviewing a PR (this one https://github.com/apache/geode/pull/3197) >> and made a note that maybe we should remove comments that make references >> to bug and trac numbers that people cannot reach (like me for one). Kirk >> mentioned that some people (like him) have access to those bugs and have >> proven helpful for some history. So there is this balance between noise >> (people who cannot access those old issues) and getting context (people who >> can access those issues). >> >> So I guess my point is to start a discussion on what a path forward might >> be (if any)? My opinion is that they are noise and we should remove them. >> If someone has access to the original issue, then making sure there is a >> test case covering it should be done. Then it makes even more sense to me >> to remove the comment. >> >> -michael
Re: Remove usages of deprecated classes
I agree that we have too many uses of code deprecated in our own code base. I also agree with the idea that we should not introduce new usages of deprecated classes. So if someone is modifying a class that uses deprecated classes, should the deprecations be refactored out or is it OK to leave them? Seems hard to make firm “rules” (or should we call them guidelines?). Each case needs to be looked at with respect to code complexity, how extensive is the original code change and are the deprecated uses in the area of change, and will removing the deprecations require API changes to inject new dependencies to support removing the deprecations. We do have the guideline that deprecated classes can’t be removed until at least the major release after the release in which the class was deprecated. So currently we’re looking at Geode 2.0 to possibly remove deprecated classes. Now to get where it would be OK to remove a deprecated class in 2.0.0, then it seems to me that the initiative should be to remove all current uses of the class in the core product and in test code prior to when the decision is made to work toward a 2.0.0 release. In other words, do it sooner and avoid the rush to do it all just before the release. > On Jan 2, 2019, at 11:38 AM, Peter Tran wrote: > > Hello Geode Dev, > > As a new contributor reviewing PRs I've learnt that it's acceptable to make > a PR that continues to use deprecated classes but not okay to introduce the > usage of a deprecated class. > > I wonder if there should be a systematic way to remove the usage of > deprecated classes. I'm concerned over time the code base will accumulate > more and more deprecated classes unless there is a system in place in which > they are removed. > > Has there been any initiatives in the past to do this? Are having a lot of > deprecated classes still in use a low risk thing? > > Thanks, > > Peter > -- > Peter Tran > PCF Toronto
Re: Javadoc errors in org/apache/geode/cache/configuration/RegionAttributesType.java
I merged that for you this earlier this morning > On Dec 20, 2018, at 4:10 PM, Aditya Anchuri wrote: > > Okay. Please go ahead and merge, I can’t since I’m not a committer. > > On Thu, Dec 20, 2018 at 3:00 PM Kirk Lund wrote: > >> Looks good. Thanks! I added my approval. >> >> On Thu, Dec 20, 2018 at 2:20 PM Aditya Anchuri >> wrote: >> >>> PR to fix: https://github.com/apache/geode/pull/3030 >>> >>> On Thu, Dec 20, 2018 at 1:58 PM Aditya Anchuri >>> wrote: >>> These classes were removed recently, but it looks like we forgot to >> clean up the javadocs. Will make a PR to fix ASAP. -Aditya On Thu, Dec 20, 2018 at 1:57 PM Kirk Lund wrote: > Just a reminder that any classes not behind internal packages need to >>> have > functional Javadocs. > > We have Javadoc errors involving > org/apache/geode/cache/configuration/RegionAttributesType.java >> referring > to > RegionTimeToLive which I can't find when I grep. > >> Task :geode-assembly:docs > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:479: > warning - Tag @link: reference not found: > RegionAttributesType.RegionTimeToLive > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:490: > warning - Tag @link: reference not found: > RegionAttributesType.RegionTimeToLive > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:501: > warning - Tag @link: reference not found: > RegionAttributesType.RegionIdleTime > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:512: > warning - Tag @link: reference not found: > RegionAttributesType.RegionIdleTime > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:523: > warning - Tag @link: reference not found: > RegionAttributesType.EntryTimeToLive > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:534: > warning - Tag @link: reference not found: > RegionAttributesType.EntryTimeToLive > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:545: > warning - Tag @link: reference not found: > RegionAttributesType.EntryIdleTime > > >>> >> /Users/klund/dev/gemfire1/open/geode-core/src/main/java/org/apache/geode/cache/configuration/RegionAttributesType.java:556: > warning - Tag @link: reference not found: > RegionAttributesType.EntryIdleTime > 8 warnings > >>> >>
Review Request 62248: GEODE-3539: Add test for missing coverage of status locator command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62248/ --- Review request for geode, Jinmei Liao, Jared Stewart, and Patrick Rhomberg. Repository: geode Description --- New test to replace a legacy test being removed. Removes some commented out code from other status locator command tests Diffs - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsTest.java bc309dd3111addebac61305b3e7223bc1ade78f3 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java dd5841f4cffca38da07a11f381cf4174d7264349 Diff: https://reviews.apache.org/r/62248/diff/1/ Testing --- precheckin passed Thanks, Ken Howe
Re: Review Request 62189: GEODE-2817: consolidate authorize(*) methods
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62189/#review185028 --- geode-core/src/main/java/org/apache/geode/cache/execute/Function.java Lines 120 (patched) <https://reviews.apache.org/r/62189/#comment261259> You are expecting an internal object to be passed in a public API here. I think this can be deleted from the current change set as in my search it doesn't appear to be used. - Ken Howe On Sept. 8, 2017, 4:28 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62189/ > --- > > (Updated Sept. 8, 2017, 4:28 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2817: consolidate authorize(*) methods > > * this is just a changeset that removed those authorizeCluster*, > authorizeData* authorizeRegion* functions. They made the interface > unnecessaily large, and doesn't really offer much value. I prefer to see > authorize(DATA, READ, regionname), insteadof authorizeRegionRead(*) in the > code so that it's clear what exactly is the permssion needed. This also makes > the further change/update to the interface easier to manage. > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/cache/execute/Function.java > 25ba4e37f579e1f222439ef19c223111faabda61 > > geode-core/src/main/java/org/apache/geode/cache/execute/FunctionContext.java > 0b4e7f9de38b234742a427b55ceda2e63d93d83e > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/auth/GMSAuthenticator.java > f895b964794f99127f1f0c9564f3f85213e0af22 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommandQuery.java > 184aa36fc5509285001155e20d846cc717544d2f > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ClearRegion.java > 610af436cd96b0663d69915d8a1b37549e4f7bc2 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ContainsKey.java > d7a1b2b0183405142d524c1d91433eb01ac3e9f6 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ContainsKey66.java > 03e798c579e6c26e3aa7cb9a522f8af514f60ce3 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CreateRegion.java > 2be4724bc2b2b6d472558cdecddba982da032a08 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Destroy.java > cdbab8047558234bd25aa6155a7603a62d697d03 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Destroy65.java > c8b794a9b0b4c223e6391a8e7f63aa0747943417 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/DestroyRegion.java > baa2f3f480ce4bea522ea520e86328384cfd2d23 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteFunction.java > 08f02642e937abf3b0e2557cd06c8264ac0dee32 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteFunction65.java > 53db561963d7b9abbe86e0dbf7e73613f6a5ceef > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteFunction66.java > a3b061f761a9f1e6a7302b5e8dc31e9a98a9959c > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunction.java > 73eff40c34f6fe882ec1e21168e522ae9ced2736 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunction65.java > 47684aa2fb5d8ec0190f0e047a627c08e77598f1 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunction66.java > 26d5d3f31a6b0cf2e2ff1477a2e5e3449fd5453d > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteRegionFunctionSingleHop.java > 8c3bb381fe6e2e9018aaf8c333a3ab0c26127c27 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Get70.java > 62644eb109f661a00c2c4df06e96c8d7a4f8d33f > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetAll.java > e214ce16e4be1f044f40f44308435eec43d1c8ca > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetAll651.java > aacfc6dccd6385ac39a9c2b65ecf6f7456bba704 > > geode-core/src/main/java/org/apa
Re: 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/ --- (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 62179: Test Rule Fix: clean up client DS when using LocatorServerStartupRule
> On Sept. 8, 2017, 4:37 p.m., Jared Stewart wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > > Line 107 (original), 107 (patched) > > <https://reviews.apache.org/r/62179/diff/1-2/?file=1818168#file1818168line109> > > > > I like this use of `IntStream`! +1 Much clearer to read > On Sept. 8, 2017, 4:37 p.m., Jared Stewart wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > > Lines 232 (patched) > > <https://reviews.apache.org/r/62179/diff/1-2/?file=1818168#file1818168line243> > > > > Do you think there is any chance someone will start both a server and a > > client in the same VM? (To deal with that I think we would just remove the > > `else{` and always try to disconnect the DS, even if `member` was not > > `null`) > > Jinmei Liao wrote: > the member.stopMember() eventually calls MemberStarerRule.stopMember() > which will call disconnectDS already. Is it still necessary to this? > > Jared Stewart wrote: > Hm, maybe I'm missing something. All I see in > MemberStarterRule.stopMember() is ` abstract void stopMember();`, and it > doesn't look like either of the concrete implementations (in > LocatorStarterRule and ServerStarterRule) explicitly disconnect the DS. > Perhaps `InternalLocator.stop()` and `CacheServer.stop()` already disconnect > the DS? If that's the case I don't think it should be necessary for us to > disconnect it again. I agree with Jinmei, looks to me that MemberStarterRule.stopMember() eventually calls MemberStarterRule.after(), which will then disconnectDSIfAny - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62179/#review184984 --- On Sept. 8, 2017, 3:51 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62179/ > --- > > (Updated Sept. 8, 2017, 3:51 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Test Rule Fix: clean up clientVM if using LocatorServerStartUpRule to get the > ClientVM > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/IndexCommandsShareConfigurationDUnitTest.java > 1a5fc3485e159ca311247e617d5bec2b37c6ee10 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowMissingDiskStoresDUnitTest.java > 3e9227e99ad57624b024498d0ff5e807c8853221 > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ImportClusterConfigDistributedTest.java > 31c58177d60c9cf9ad0d07fc7a7daf8b332d3c9e > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > f0385e21f708d9a085e7129d82fb3101e2fb8322 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java > a832a2590527100afc05fb9de2e332a263d52c19 > > geode-core/src/test/java/org/apache/geode/test/dunit/standalone/DUnitLauncher.java > b35270e2d97930cee68d8c54221a04c20dfb96de > > geode-wan/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java > c301d587c04651a03170e3da6451ebf2acf063c0 > > > Diff: https://reviews.apache.org/r/62179/diff/2/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: 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/ --- (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
> 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 > >
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 61972: GEODE-3445: Convert connect acceptance test to DUnit test
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61972/#review184156 --- Ship it! geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithSSLTest.java Line 267 (original), 264 (patched) <https://reviews.apache.org/r/61972/#comment260207> Spelling: "conect" corrected elsewhere, but missed this one. - Ken Howe On Aug. 29, 2017, 4:53 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61972/ > --- > > (Updated Aug. 29, 2017, 4:53 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3445: Convert connect w/ http & ssl acceptance test to DUnit test > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/management/GfshConnectToLocatorWithSSLAcceptanceTest.java > 75d60a32411582d75eb0f5cacce1536a6f724a26 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandWithSSLTest.java > 7c4fb446ca4909c628010087b7c85aa121883894 > > > Diff: https://reviews.apache.org/r/61972/diff/1/ > > > Testing > --- > > Precheckin passed > > > Thanks, > > Jared Stewart > >
Re: Review Request 61974: GEODE-2859: Fix race condition in ShowDeadlockDUnitTest
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61974/#review184151 --- Ship it! Ship It! - Ken Howe On Aug. 29, 2017, 10:46 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61974/ > --- > > (Updated Aug. 29, 2017, 10:46 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2859: Fix race condition in ShowDeadlockDUnitTest > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java > 4df0b96 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > dc17d03 > geode-junit/build.gradle 7c533ad > > geode-junit/src/main/java/org/apache/geode/test/concurrent/FileBasedCountDownLatch.java > PRE-CREATION > > geode-junit/src/test/java/org/apache/geode/test/concurrent/FileBasedCountDownLatchTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61974/diff/2/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 61701: 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/61701/ --- (Updated Aug. 24, 2017, 10:02 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Additional changes in the launchers due to tests that were failing from expected/actual mismatches of localhost address in LocatorLauncherRemoteIntegrationTest and ServerLauncherRemoteIntegrationTest. Fixed a bug in LocatorLauncher.LocatorState.getBindAddressAsString(), apparently from when legacy code was open sourced and geode's StringUtils was replaced by the StringUtils in Apache Commons. (!StringUtils.isBlank) was at some point copy/pasted incorrectly without the '!'. Updated tests ```*LauncherRemoteIntegrationTest``` to take into account the updated information in the LocatorState & ServerState classes. Repository: geode Description --- Updated tests for changes in the error constructors for ServerState and LocatorState. Minor spelling corrections. Diffs (updated) - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java 3a98373938e3de21da6badcf460dae3648218ac6 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/test/java/org/apache/geode/distributed/LauncherIntegrationTestCase.java 409a96dbe416a6f96c2389356b9d823d1adb793f geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java cc42a53772f3064b800ca1ac1ae894be6c715399 geode-core/src/test/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java 733a1082ae9993fbdb646712380af7dcc1cca560 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java dd5841f4cffca38da07a11f381cf4174d7264349 geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java e7f17ef208a1708f385c7c4041affb70fd309a4c Diff: https://reviews.apache.org/r/61701/diff/5/ Changes: https://reviews.apache.org/r/61701/diff/4-5/ Testing (updated) --- Precheckin from earlier ran green. 8/23/17: Re-running precheckin with this additional refactoring. 8/24/17: Once more around the precheckin merry-go-round Thanks, Ken Howe
Re: Review Request 61701: 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/61701/ --- (Updated Aug. 23, 2017, 8:43 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Code cleanup -remove trailing blanks -remove unused imports Repository: geode Description --- Updated tests for changes in the error constructors for ServerState and LocatorState. Minor spelling corrections. Diffs (updated) - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java 3a98373938e3de21da6badcf460dae3648218ac6 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/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java dd5841f4cffca38da07a11f381cf4174d7264349 geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java e7f17ef208a1708f385c7c4041affb70fd309a4c Diff: https://reviews.apache.org/r/61701/diff/4/ Changes: https://reviews.apache.org/r/61701/diff/3-4/ Testing --- Precheckin from earlier ran green. Re-running precheckin with this additional refactoring. Thanks, Ken Howe
Re: Review Request 61701: 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/61701/ --- (Updated Aug. 22, 2017, 9:28 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Refactored ServerLauncher similar to the previous change to LocatorLauncher to remove nested ternary expressions. Repository: geode Description --- Updated tests for changes in the error constructors for ServerState and LocatorState. Minor spelling corrections. Diffs (updated) - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java 3a98373938e3de21da6badcf460dae3648218ac6 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/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java dd5841f4cffca38da07a11f381cf4174d7264349 geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java e7f17ef208a1708f385c7c4041affb70fd309a4c Diff: https://reviews.apache.org/r/61701/diff/3/ Changes: https://reviews.apache.org/r/61701/diff/2-3/ Testing (updated) --- Precheckin from earlier ran green. Re-running precheckin with this additional refactoring. Thanks, Ken Howe
Re: Gitbox enables the full GitHub workflow
+1 Yes, let’s make the move > On Aug 22, 2017, at 11:21 AM, Nabarun Nagwrote: > > +1 > > On Tue, Aug 22, 2017 at 11:15 AM Kirk Lund wrote: > >> +1 to move all our repos to gitbox >> >> On Tue, Aug 22, 2017 at 11:08 AM, Jacob Barrett >> wrote: >> >>> +1 >>> >>> Sent from my iPhone >>> On Aug 22, 2017, at 10:49 AM, Jared Stewart >> wrote: +1 for moving the other repos to Gitbox On Aug 22, 2017 10:43 AM, "Anthony Baker" wrote: > On Aug 7, 2017, at 6:09 PM, Roman Shaposhnik >>> wrote: > > Hi! > > it has just come to my attention that Gitbox at ASF > has been enabling full GitHub workflow (with being > able to click Merge this PR button, etc.) for quite > some time. > > This basically allows a project to have GH as a R/W > repo as opposed to R/O mirror of what we all currnently > have: https://gitbox.apache.org/repos/asf > > Personally I'm not sure I like GH workflow all that much, > but if there's interest -- you can opt-in into Gitbox. Once > you do -- your source of truth moves to GH. You can't > have it both ways with git-wip-us.apache.org and Gitbox. > > Thanks, > Roman. Now that we’ve got some experience with gitbox on geode-native, are we ready to convert the other repos? - geode - geode-site - geode-examples I think we should. Anthony >>> >>
Re: Review Request 61701: 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/61701/ --- (Updated Aug. 22, 2017, 6:26 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Refactored ```LocatorLauncher.LocatorState``` constructor as suggested, removing nested ternary expressions. Repository: geode Description (updated) --- Updated tests for changes in the error constructors for ServerState and LocatorState. Minor spelling corrections. Diffs (updated) - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java 3a98373938e3de21da6badcf460dae3648218ac6 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/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java dd5841f4cffca38da07a11f381cf4174d7264349 geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java e7f17ef208a1708f385c7c4041affb70fd309a4c Diff: https://reviews.apache.org/r/61701/diff/2/ Changes: https://reviews.apache.org/r/61701/diff/1-2/ Testing (updated) --- Re-running precheckin Thanks, Ken Howe
Re: Review Request 61701: GEODE-3277: Fix error path constructors of Launcher inner State classess
> On Aug. 17, 2017, 5:35 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java > > Lines 2017 (patched) > > <https://reviews.apache.org/r/61701/diff/1/?file=1798850#file1798850line2020> > > > > I think this double-ternary might be easier to read if we split it up > > into a few separate statements. Agree with you that the double ternary sin't the most readable construct. However, I ended up using it because the `this()` call has to be the first statement in the constructor. The conditionals guard against NPEs from the possible locations where this error constructor is called. I'm open to other refactoring suggestions to make this easier to understand. > On Aug. 17, 2017, 5:35 p.m., Jared Stewart wrote: > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java > > Lines 85 (patched) > > <https://reviews.apache.org/r/61701/diff/1/?file=1798852#file1798852line85> > > > > Looks like you intended to uncomment this line. Acutally I forgot to delete this line. I found that some fields from `LocatorStatusResponse` (e.g.`logFile`) that are included in the `GFJsonObject` in `CommandResult` contain the string `"null"`. The intent of this test is to insure that the status command correctly shows the locator is online when given the correct `--port=...` value. - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61701/#review183145 --- On Aug. 16, 2017, 9:21 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61701/ > --- > > (Updated Aug. 16, 2017, 9:21 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. > > I've updated the fix for GEODE-3277 and rebased on top of the @klund's recent > process controller updates > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java > 3a98373938e3de21da6badcf460dae3648218ac6 > 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/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java > dd5841f4cffca38da07a11f381cf4174d7264349 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > e7f17ef208a1708f385c7c4041affb70fd309a4c > > > Diff: https://reviews.apache.org/r/61701/diff/1/ > > > Testing > --- > > Precheckin is in progress. > > > Thanks, > > Ken Howe > >
Review Request 61701: 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/61701/ --- 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. I've updated the fix for GEODE-3277 and rebased on top of the @klund's recent process controller updates Diffs - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java 3a98373938e3de21da6badcf460dae3648218ac6 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/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java dd5841f4cffca38da07a11f381cf4174d7264349 geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java e7f17ef208a1708f385c7c4041affb70fd309a4c Diff: https://reviews.apache.org/r/61701/diff/1/ Testing --- Precheckin is in progress. Thanks, Ken Howe
Re: Review Request 61627: GEODE-3437: Fix list and describe region tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61627/#review183042 --- Ship it! Ship It! - Ken Howe On Aug. 16, 2017, 5:24 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61627/ > --- > > (Updated Aug. 16, 2017, 5:24 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3437: Fix list and describe region tests > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListAndDescribeRegionDUnitTest.java > ab8c69b > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > fc7966f > > > Diff: https://reviews.apache.org/r/61627/diff/2/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 61627: GEODE-3437: Fix list and describe region tests
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61627/#review183037 --- Ship it! Ship It! - Ken Howe On Aug. 14, 2017, 10:40 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61627/ > --- > > (Updated Aug. 14, 2017, 10:40 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3437: Fix list and describe region tests > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ListAndDescribeRegionDUnitTest.java > ab8c69b7cc99c88dd4e928efeb441d7d1a1d9b1b > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > fc7966f5eb2a9ca4c30369a20ce664d3929ecc22 > > > Diff: https://reviews.apache.org/r/61627/diff/1/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 61671: GEODE-3328: fix testAddGemFirePropertyFileToCommandLineNew on Windows
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61671/#review182999 --- Ship it! geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandJUnitTest.java Line 411 (original), 411 (patched) <https://reviews.apache.org/r/61671/#comment258982> Why was this test renamed? Not really a problem but on the surface looks unneeded. - Ken Howe On Aug. 15, 2017, 7:29 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61671/ > --- > > (Updated Aug. 15, 2017, 7:29 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, and Ken Howe. > > > Bugs: GEODE-3328 > https://issues.apache.org/jira/browse/GEODE-3328 > > > Repository: geode > > > Description > --- > > I had to temporarily revert Jinmei's fix for GEODE-3328 in order to fully > revert Emily's refactorings of GFSH commands. This commit reintroduces her > fix with minor tweak to get it to work without Emily's changes. > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandJUnitTest.java > da60c7aa481954be0acc8c7e2b88717cf8bc9c37 > > > Diff: https://reviews.apache.org/r/61671/diff/1/ > > > Testing > --- > > precheckin in progress > > > Thanks, > > Kirk Lund > >
Re: Review Request 61599: GEODE-3328: fix a test failure on windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61599/#review182758 --- Ship it! Replacing hardcoded path strings with system independant constructs is always good. I didn't try this out myself on a Windows machine but the fix looks good. - Ken Howe On Aug. 11, 2017, 10:42 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61599/ > --- > > (Updated Aug. 11, 2017, 10:42 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3328: fix a test failure on windows. > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GfshCommandJUnitTest.java > da60c7aa481954be0acc8c7e2b88717cf8bc9c37 > > > Diff: https://reviews.apache.org/r/61599/diff/1/ > > > Testing > --- > > the test itself since only this test is changed. > > > Thanks, > > Jinmei Liao > >
Re: Review Request 61480: GEODE-3330: user needs CLUSTER:MANAGE:QUERY permission to create a CQ.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61480/#review182443 --- Ship it! Ship It! - Ken Howe On Aug. 8, 2017, 9:10 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61480/ > --- > > (Updated Aug. 8, 2017, 9:10 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > * cq.execute() and cq.executeWithInitialResult() all would still require > DATA:READ because it will send the result back to the client either initially > or later. > > > Diffs > - > > geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java > 5d5c2148e2eba8054df305942e04f43ea69c0a79 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > a455aff315cd26abd398630ff63d8b54b0d1d12b > > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java > 6748f7d3193babfa668a7ff2846f974f0cdc1cbd > > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java > 77a608c7e65a2030c0037e9f327cf8c17e9313db > > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQ.java > a3d51edcc141391e9d818fc0ed7e514d3cb5d6d0 > > geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQTest.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61Test.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQTest.java > PRE-CREATION > geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java > 89167258ddbc06315068c849255a8530faefe060 > > geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java > 45f45abd17cf4f90f96163ebe4f01e67b3b53633 > geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/security/ClientQueryAuthDUnitTest.java > cc5dde409c561522ae3739eeaee892079c509ae8 > > geode-cq/src/test/java/org/apache/geode/test/dunit/rules/CQUnitTestRule.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61480/diff/2/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 61480: GEODE-3330: user needs CLUSTER:MANAGE:QUERY permission to create a CQ.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61480/#review182440 --- Ship it! - Ken Howe On Aug. 8, 2017, 9:10 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61480/ > --- > > (Updated Aug. 8, 2017, 9:10 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > * cq.execute() and cq.executeWithInitialResult() all would still require > DATA:READ because it will send the result back to the client either initially > or later. > > > Diffs > - > > geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java > 5d5c2148e2eba8054df305942e04f43ea69c0a79 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > a455aff315cd26abd398630ff63d8b54b0d1d12b > > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java > 6748f7d3193babfa668a7ff2846f974f0cdc1cbd > > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java > 77a608c7e65a2030c0037e9f327cf8c17e9313db > > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQ.java > a3d51edcc141391e9d818fc0ed7e514d3cb5d6d0 > > geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQTest.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61Test.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQTest.java > PRE-CREATION > geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java > 89167258ddbc06315068c849255a8530faefe060 > > geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java > 45f45abd17cf4f90f96163ebe4f01e67b3b53633 > geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/security/ClientQueryAuthDUnitTest.java > cc5dde409c561522ae3739eeaee892079c509ae8 > > geode-cq/src/test/java/org/apache/geode/test/dunit/rules/CQUnitTestRule.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61480/diff/2/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 61487: GEODE-3407: fix deadlock between JMX and reconnect
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61487/#review182391 --- geode-core/src/main/java/org/apache/geode/management/internal/beans/CacheServerBridge.java Lines 114-115 (patched) <https://reviews.apache.org/r/61487/#comment258280> To avoid any confusion, I suggest that the order of the cache and cacheServer parameters be the same for this and the preceding constructor. I prefer the order decalred here, (cache, cacheServer, ...) - Ken Howe On Aug. 8, 2017, 12:19 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61487/ > --- > > (Updated Aug. 8, 2017, 12:19 a.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, > and Patrick Rhomberg. > > > Bugs: GEODE-3407 > https://issues.apache.org/jira/browse/GEODE-3407 > > > Repository: geode > > > Description > --- > > Change InternalClientMembership to not synchronize on CacheFactory > by accepting Cache parameter from CacheServerBridge. > > New regression test confirms bug and this fix. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/InternalClientMembership.java > 504081d65515adb294dd43ecffee450477f08339 > > geode-core/src/main/java/org/apache/geode/management/internal/beans/CacheServerBridge.java > 728402c8a290d88026db753657e26e5f7440a990 > > geode-core/src/main/java/org/apache/geode/management/internal/beans/ServerBridge.java > 6834998da13deec5e074a61e5373ec2f8dce2ad7 > > geode-core/src/test/java/org/apache/geode/management/internal/beans/CacheServerBridgeClientMembershipRegressionTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/61487/diff/1/ > > > Testing > --- > > precheckin in progress > > > Thanks, > > Kirk Lund > >
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to the config
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61417/#review182233 --- geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java Lines 691 (patched) <https://reviews.apache.org/r/61417/#comment258115> Is this declaration needed? It doesn't appear to be used anywhere - Ken Howe On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61417/ > --- > > (Updated Aug. 3, 2017, 9:15 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, > Patrick Rhomberg, and Udo Kohlmeyer. > > > Repository: geode > > > Description > --- > > GEODE-3328: adding ssl-truststore-type to the config > > this is what's requested in the JIRA ticket. This changeset just deals with > adding the property into gemfire properties > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java > 63f6505101f6edb62167b854d3d16d76d0a893cd > > geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java > 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8 > > geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java > c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f > > geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java > fbe894c96447b2e30594eb2ed0652dd08e1028ce > > geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java > f86f07eb5e58b8509e909b7748795530efd65339 > geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java > 08fa9b54ea066b4158478ae89d8295ed0b1a337b > > geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java > 499ef010f354ebf88765190f1b5eb945da83accc > > geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java > 525f988cd3cb4f19872168df9b7105645f5c0498 > > > Diff: https://reviews.apache.org/r/61417/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 61409: GEODE-3328: simplify GfshParserRule
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61409/#review182227 --- Ship it! Ship It! - Ken Howe On Aug. 3, 2017, 5:12 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61409/ > --- > > (Updated Aug. 3, 2017, 5:12 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3328: simplify GfshParserRule > > another step towards refactor connect command, some simple rule change. > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommandIntegrationTest.java > 0700742fac70730c160d28c62c93b824e8ee0a3c > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StartServerCommandIntegrationTest.java > 059611dc1c5101643cbae18d067a2943a573d405 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsTest.java > c2810801257479ad9a31452f294daaaf2975dfad > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshParserRule.java > b1bc7aa73b7d9273ad9a89af4c119d91a67aae03 > > > Diff: https://reviews.apache.org/r/61409/diff/1/ > > > Testing > --- > > precheckin running, tests > > > Thanks, > > Jinmei Liao > >
Review Request 61426: GEODE-3277: Fix error path constructors of inner State classes of the Launchers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61426/ --- 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 refactorings and spelling corrections to improve code clarity. Some error results from gfsh 'status locator' and 'server loccator' commands returned a messge of "null", or in some cases woudl throw a NumberFormatException. These results were due to constructors for LocatorLauncher.LocatorState and ServerLauncher.ServerState filling relevant fields with nulls. This change sets the fields in the ...State instances to values available through the command being executed. The affect constructors are only used for building error results for the status commands. Diffs - geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java 3a98373938e3de21da6badcf460dae3648218ac6 geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java c5a2de88086e92dfc9b35d764b88ff8c8e524853 geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java 158e7bf45a3cb72f6b96345810b935096b44ee7e geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StatusLocatorCommand.java 06f835034c32e7c6cc7a11d9657d8b5d40d0f2d8 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/lifecycle/StatusServerCommand.java 43374ab161b67357d2f8b2987d7656156cbc12c1 geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java 47e512a2dc3a8780dd941af8309865c4f1dbf36f geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/lifecycle/GfshStatusCommandsIntegrationTest.java PRE-CREATION geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java e7f17ef208a1708f385c7c4041affb70fd309a4c Diff: https://reviews.apache.org/r/61426/diff/1/ Testing --- Precheckin ran green Thanks, Ken Howe
Re: Review Request 61196: GEODE-3326: Fix intermittent ConcurrentDeployDUnitTest failure
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61196/#review181674 --- Ship it! Ship It! - Ken Howe On July 27, 2017, 10:22 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61196/ > --- > > (Updated July 27, 2017, 10:22 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3326: Fix intermittent ConcurrentDeployDUnitTest failure > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ConcurrentDeployDUnitTest.java > 559440c8a2f227de092e52979d9db1f959a5d75f > > > Diff: https://reviews.apache.org/r/61196/diff/1/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60985/#review181069 --- Ship it! Ship It! - Ken Howe On July 20, 2017, 5:54 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60985/ > --- > > (Updated July 20, 2017, 5:54 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3217: Reimplement gfsh query as a single-step command > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java > 3f4397b > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java > fe88fc9 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java > 41cc171 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java > 8ab7c93 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java > fb63184 > > geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java > e5d6ce8 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60985/diff/2/ > > > Testing > --- > > Precheckin passed (except for some flaky failures that appear to be unrelated) > > > Thanks, > > Jared Stewart > >
Re: Review Request 60985: GEODE-3217: Reimplement gfsh query as a single-step command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60985/#review181043 --- geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java Lines 15 (patched) <https://reviews.apache.org/r/60985/#comment256477> Extra blank line. Check this in all of the new class files. geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java Lines 24 (patched) <https://reviews.apache.org/r/60985/#comment256480> Seems there's opportunity for more tests in here, for instance, queryWithInvalidRegionName, queryInvalidExceptionThrown, etc. Have you checked coverage, in particular for the new classes QueryCommand, and QueryInterceptor - Ken Howe On July 19, 2017, 9:06 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60985/ > --- > > (Updated July 19, 2017, 9:06 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3217: Reimplement gfsh query as a single-step command > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java > 3f4397b3d025ee5453a6df338eed41589ff339c0 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryCommand.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueryInterceptor.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java > fe88fc98d397fa748f8f54b900b1511eb114c0b6 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java > 41cc171dbabee2cf51a9e28d051b8365a3c44699 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java > 8ab7c9342ebd5370deba45a35dbf68201f2b7333 > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DataCommandsController.java > fb6318449e027c94ba4fd2fa87c84f8181ce61a4 > > geode-core/src/test/java/org/apache/geode/management/DataCommandMBeanTest.java > e5d6ce8d5bfb35eb503c66c9650fde2a33a51315 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/QueryCommandUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/result/ResultBuilderTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60985/diff/1/ > > > Testing > --- > > Precheckin passed (except for some flaky failures that appear to be unrelated) > > > Thanks, > > Jared Stewart > >
Re: Review Request 60977: GEODE-3251: make JMX test rules more robust
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60977/#review180959 --- Ship it! Ship It! - Ken Howe On July 19, 2017, 5:14 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60977/ > --- > > (Updated July 19, 2017, 5:14 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, > and Patrick Rhomberg. > > > Bugs: GEODE-3251 > https://issues.apache.org/jira/browse/GEODE-3251 > > > Repository: geode > > > Description > --- > > GEODE-3251: make JMX test rules more robust > > Add "Failed to retrieve RMIServer stub" to rule await > to help ensure that JMX RMI is ready before running > the test. > > Also removed Soft assertions from DiskStoreMXBeanSecurityJUnitTest. > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java > 4d101e7862dd45bbf4faaec84cb4f808cfd26cf3 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 872553efed290890708caaf98dc206b4735005d0 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MBeanServerConnectionRule.java > 3addc4e2ff07d36a340ed548e4c9a2d7f6094390 > > > Diff: https://reviews.apache.org/r/60977/diff/1/ > > > Testing > --- > > DiskStoreMXBeanSecurityJUnitTest > precheckin in progress > > > Thanks, > > Kirk Lund > >
Re: Review Request 60666: geode-3166: remove the uncalled getCredential method
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60666/#review180683 --- Ship it! Ship It! - Ken Howe On July 17, 2017, 3:49 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60666/ > --- > > (Updated July 17, 2017, 3:49 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > geode-3166: remove the uncalled getCredential method > > I simply removed the call. Since the new method is never called, if we have > any users that's implementing this method, it's never used anyway. We will > not be breaking backward compatibility, I believe. > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/security/AuthInitialize.java > 6b0675a5629a3e057fd3d54841db740189007966 > > > Diff: https://reviews.apache.org/r/60666/diff/3/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 60666: geode-3166: remove the uncalled getCredential method
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60666/#review179785 --- For the record, and as discussed offline, I think 1-arg method should be marked as @Deprecated for the upcoming release rather than immediately removing it. Removing the @Deprecated annotation on the 3-arg method is appropriate as this is now the preferred method. - Ken Howe On July 5, 2017, 7:47 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60666/ > --- > > (Updated July 5, 2017, 7:47 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > geode-3166: remove the uncalled getCredential method > > I simply removed the call. Since the new method is never called, if we have > any users that's implementing this method, it's never used anyway. We will > not be breaking backward compatibility, I believe. > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/security/AuthInitialize.java > 6b0675a5629a3e057fd3d54841db740189007966 > > > Diff: https://reviews.apache.org/r/60666/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 60348: GEODE-3103: GfshRule no longer clutters output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60348/#review178811 --- Ship it! LGTM geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java Line 103 (original), 102 (patched) <https://reviews.apache.org/r/60348/#comment253035> Yeah, That's even better than my suggestion! - Ken Howe On June 23, 2017, 6:01 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60348/ > --- > > (Updated June 23, 2017, 6:01 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > - GfshRule writes via a logger rather than StdOut. This will make it no > longer clutter precheckin runs or the nightly build. > - Introduce ProcessLogger to capture output from the Gfsh JVM so that tests > can assert against the output. > > > Diffs > - > > geode-assembly/build.gradle 39bb542 > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java > 82ee240 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java > 8109377 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshScript.java > 3ee1402 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/StreamGobbler.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java > cfb8a24 > > > Diff: https://reviews.apache.org/r/60348/diff/3/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 60348: GEODE-3103: GfshRule no longer clutters output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60348/#review178703 --- geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java Lines 103 (patched) <https://reviews.apache.org/r/60348/#comment252855> I've always disliked writing loops with offsets from the loop counter used within the loop body, they make you pause and reason out whether the index is correct. I think something like ``` int i=1; for (String cmd:specifiedCommands) { commandsToExecute[i++] = "-e " + cmd; } ``` reads a bit better, although it does separate the loop iterator and the index counter. geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java Line 56 (original), 59 (patched) <https://reviews.apache.org/r/60348/#comment252860> Probably should only add the enclosing '"' on `value` if the string doesn't already have them. A quick grep showed 3 tests (IndexCOmmandsDUnitTest) that have enclosing quotes on the value arg. -OR- change those tests to be consistent with adding the quotes here. - Ken Howe On June 21, 2017, 10:51 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60348/ > --- > > (Updated June 21, 2017, 10:51 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > - GfshRule writes via a logger rather than StdOut. This will make it no > longer clutter precheckin runs or the nightly build. > - Introduce ProcessLogger to capture output from the Gfsh JVM so that tests > can assert against the output. > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java > 82ee240539b0190a8698939faa63c696d1e03efb > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshExecution.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java > 81093778d379d6610b94ee93b180bcc773eb3595 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshScript.java > 3ee1402aa9c390d1bc097e29190010ff1496fca6 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/ProcessLogger.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/StreamGobbler.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/CommandStringBuilder.java > cfb8a24d250af11723c27d35112f6a5e4fde4568 > > > Diff: https://reviews.apache.org/r/60348/diff/1/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 60200: GEODE-3095: fix parameter type mismatch between the diskstore command and controller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60200/#review178313 --- Ship it! Ship It! - Ken Howe On June 19, 2017, 9:45 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60200/ > --- > > (Updated June 19, 2017, 9:45 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3095: fix parameter type mismatch between the diskstore command and > controller > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DiskStoreCommandsController.java > c613a8a1d5cca93ce7be785b58f6502d96752d8c > > geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java > ea4b60d4ec72def55129706d7e4d8f297d87ddcb > > > Diff: https://reviews.apache.org/r/60200/diff/3/ > > > Testing > --- > > GfshCommandOverHttpTest, GfshSecurityCommandOverHttpTest > > > Thanks, > > Jinmei Liao > >
Re: Review Request 60200: GEODE-3095: fix parameter type mismatch between the diskstore command and controller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60200/#review178305 --- Ship it! Ship It! - Ken Howe On June 19, 2017, 9:12 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60200/ > --- > > (Updated June 19, 2017, 9:12 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3095: fix parameter type mismatch between the diskstore command and > controller > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/DiskStoreCommandsController.java > c613a8a1d5cca93ce7be785b58f6502d96752d8c > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CreateDiskStoreOverHttpTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60200/diff/2/ > > > Testing > --- > > GfshCommandOverHttpTest, GfshSecurityCommandOverHttpTest > > > Thanks, > > Jinmei Liao > >
Re: Review Request 60202: GEODE-3056: fix the message for invalid partition-resolver
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60202/#review178302 --- Ship it! Ship It! - Ken Howe On June 19, 2017, 7:08 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60202/ > --- > > (Updated June 19, 2017, 7:08 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3056: fix the message for invalid partition-resolver > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java > 842802ba7b7ed34aa52974dcf327015051f22e1b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java > 47d150d180dada8066f1e644b293c56096b2a969 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommandsTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60202/diff/1/ > > > Testing > --- > > newly added test and precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 60199: GEODE-3092: fix specifiedDefaultValue for cacheLoader and cacheWriter
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60199/#review178272 --- Ship it! Ship It! - Ken Howe On June 19, 2017, 4:09 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60199/ > --- > > (Updated June 19, 2017, 4:09 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3092: fix specifiedDefaultValue for cacheLoader and cacheWriter > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java > 842802ba7b7ed34aa52974dcf327015051f22e1b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java > 47d150d180dada8066f1e644b293c56096b2a969 > > > Diff: https://reviews.apache.org/r/60199/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 60010: GEODE-3062: replace SecurityService after loading cluster config
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60010/#review177928 --- Ship it! Ship It! - Ken Howe On June 13, 2017, 4:29 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60010/ > --- > > (Updated June 13, 2017, 4:29 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, > and Patrick Rhomberg. > > > Bugs: GEODE-3062 > https://issues.apache.org/jira/browse/GEODE-3062 > > > Repository: geode > > > Description > --- > > Add new test to ClusterConfigWithSecurityDUnitTest that fails due to bug > GEODE-3062. > > Remove unused Cache param from applyClusterPropertiesConfiguration so it can > be called during Cache construction. > > Move cluster config request to Cache construction and handle jars and > properties there. Create new SecurityService in constructor and overwrite the > SecurityService in InternalDistributedSystem. > > NOTE: We will later have to fix GEODE-3061 by moving cluster config request > from Cache to InternalDistributedSystem construction so that IDS can be > created with gemfire.properties from cluster config. At that time we would > rip out both cluster config request and creation of security service from > Cache construction and pass both into Cache construction. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java > 22edb6f06c7791929cc9a033ca1a1bfed5751a47 > > geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java > 4f4881fe39116faa505bc2fbec74efd669efe0ef > > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java > 40df0c7dcac8827a381c268c1f90e6acfb97a7f1 > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java > c551ca9104a85dcf54c0bebbc4178fce4114a416 > > > Diff: https://reviews.apache.org/r/60010/diff/2/ > > > Testing > --- > > Precheckin passes > > > Thanks, > > Kirk Lund > >
Re: Review Request 60025: GEODE-3060: Introduce JUnit rule for testing the fully-assembled GFSH
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60025/#review177737 --- Ship it! Ship It! - Ken Howe On June 12, 2017, 9:50 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60025/ > --- > > (Updated June 12, 2017, 9:50 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3060: Introduce JUnit rule for testing the fully-assembled GFSH > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/StatusLocatorRealGfshTest.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshScript.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/60025/diff/2/ > > > Testing > --- > > Precheckin started (still running) > > > Thanks, > > Jared Stewart > >
Re: Review Request 59961: GEODE-3048: Introduce a rule to identify tests that require GEODE_HOME
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59961/#review177620 --- Ship it! geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/RequiresGeodeHome.java Lines 28 (patched) <https://reviews.apache.org/r/59961/#comment251315> Many of us are in the habit of putting '\n' in message strings, but I think using LINE_SEPARATOR would be better. - Ken Howe On June 9, 2017, 11:35 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59961/ > --- > > (Updated June 9, 2017, 11:35 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3048: Introduce a rule to identify tests that require GEODE_HOME > > - A subsequent commit will add this rule to all of the relevant tests. > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/RequiresGeodeHome.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59961/diff/1/ > > > Testing > --- > > > Thanks, > > Jared Stewart > >
Re: Review Request 59852: GEODE-2632: make SecurityService immutable to improve client/server performance
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59852/#review177483 --- Ship it! Ship It! - Ken Howe On June 8, 2017, 9:22 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59852/ > --- > > (Updated June 8, 2017, 9:22 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Ken Howe, > and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > See https://github.com/apache/geode/pull/450 for JMH benchmark > ClientCachePutBench. I increased the runtime of ClientCachePutBench from 30 > minutes to 2 hours to get more accurate measurement. I'll update the "Testing > Done" section with performance numbers when that finishes running. My > measurements include current develop branch and feature/GEODE-2632-21. > > The changes in this change-set involve making the SecurityService > implementation(s) immutable to improve performance of client/server as > measured by the JMH benchmark ClientCachePutBench. > > Sorry it's such a long diff again -- I tried to shrink this one down. I could > leave out the bulk of client Command classes and tests -- let me know if this > review is too big and unwieldy. > > > Diffs > - > > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/controllers/RestAPIsAndInterOpsDUnitTest.java > 45e7a6139b30c800e6096d61d1f23db36c017f99 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 38fdac692315153cda9b4956d0fbbf66fa8f6399 > > geode-core/src/main/java/org/apache/geode/cache/client/ClientCacheFactory.java > 0d678ca4d08d100ac99266c5d550d9cee7a13ea3 > geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java > e06949c94ab3dd861e9dc64089bec809b3568a6c > geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java > 4de1a95f9acdcdbc843a3412fd773c60910dc43a > > geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionManager.java > 029e6377f56d80dd81e4cec430f106ac743e5178 > > geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java > 7caad3f33ee55f72dc61c4adc2ab3de5429a1607 > > geode-core/src/main/java/org/apache/geode/distributed/internal/SecurityConfig.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/MemberFactory.java > f324e3355e77cc64a8e1cce878b3c03ad4180106 > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/MemberServices.java > 60249663aad0a59f1d292d0c5336cac33e503204 > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberFactory.java > bc94ab5afd3e6c9bb5deb9e0beed5f1f84d924fa > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/Services.java > 1404b3b6f2a44aeaf3c76b99dad8a428b1b5d1f7 > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/gms/auth/GMSAuthenticator.java > ab0ca6b4533a52528818ab3b8059576c0bd1518f > geode-core/src/main/java/org/apache/geode/internal/ClassLoadUtil.java > 60d1d3968916697ae1bbac979f6ad5f4a6ad30bd > > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractLRURegionMap.java > 988be0a72b97cf6d67dd7dd930e229f6eb867166 > > geode-core/src/main/java/org/apache/geode/internal/cache/CacheServerImpl.java > 670c697c7f6c3d22a3c79d10be4e9c9929cd612b > geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java > 67fcce85c0f812e062f602e97d604dd39c0edd9f > > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java > 5e352241ac114b891adb178e1820e8c74017fa64 > geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java > d9a34e1cfa85cc08ff4f0af81b6adc2bdbd7e869 > > geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java > 037bff6ebd0f49b7afe6b5aad4c0edd9ca3b139a > > geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/FetchEntriesMessage.java > 489ffbaddabfc2d2a46d87d3060d5c61b76c7f32 > geode-core/src/main/java/org/apache/geode/internal/cache/tier/Command.java > d7f7c7b20372351963dbc6cb1e8bab506ffcc1d0 > > geode-core/src/main/java/org/apache/geode/internal/cac
Re: Review Request 59811: GEODE-2420: add file-size-limit param to the ExportLogsController
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59811/ --- (Updated June 8, 2017, 9:08 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- As suggested removed unused imports that crept in while iterating while trying out different things during the earlier revision to the warning semantics. Repository: geode Description --- GEODE-2420: add file-size-limit param to the ExportLogsController Diffs (updated) - geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 0ff780cbf66937d8ececfb3a2d0789ee485b9b62 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 57355c0efae4c6da9470267f95e27e59aa4d8b2c geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java a369c6e1ffb330715fbde2cd69d023ed36f133ad geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java 16549e70bbebf4390bb73a481274e92ca6cad035 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 8609b3aaf0a0eb1ba903bd39c64103f9510a6a78 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java 09ee08dd6af29b9a418ef7499defc4980da787ed geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsStatsDUnitTest.java 44a036298e0991c880fc552596d296e104b97ca1 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 4e1dac013d239437829bc52dc70689c4ba15dc58 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionTest.java cc5e7d5256741ad0a48ff87c7f989a18b90f7f03 Diff: https://reviews.apache.org/r/59811/diff/4/ Changes: https://reviews.apache.org/r/59811/diff/3-4/ Testing (updated) --- 6/7/17: re-started precheckin precheckin is green with the exception of unrelated DUnit tests * re-starting precheckin once more 6/8/17: previous precheckin green except for 1 known flaky test Precheckin started Thanks, Ken Howe
Re: Review Request 59893: GEODE-3032: Fix CI failure of CommandOverHttpDUnitTest
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59893/#review177303 --- Ship it! Ship It! - Ken Howe On June 7, 2017, 11:13 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59893/ > --- > > (Updated June 7, 2017, 11:13 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3032: Fix CI failure of CommandOverHttpDUnitTest > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java > 5bbfc5b > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java > 2a6a03b > > > Diff: https://reviews.apache.org/r/59893/diff/2/ > > > Testing > --- > > Precheckin is green except for unrelated flaky DUnits. > > > Thanks, > > Jared Stewart > >
Re: Review Request 59811: GEODE-2420: add file-size-limit param to the ExportLogsController
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59811/ --- (Updated June 7, 2017, 10:34 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Updated per review Repository: geode Description --- GEODE-2420: add file-size-limit param to the ExportLogsController Diffs (updated) - geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 0ff780cbf66937d8ececfb3a2d0789ee485b9b62 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 57355c0efae4c6da9470267f95e27e59aa4d8b2c geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java a369c6e1ffb330715fbde2cd69d023ed36f133ad geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java 16549e70bbebf4390bb73a481274e92ca6cad035 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 8609b3aaf0a0eb1ba903bd39c64103f9510a6a78 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java 09ee08dd6af29b9a418ef7499defc4980da787ed geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsStatsDUnitTest.java 44a036298e0991c880fc552596d296e104b97ca1 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 4e1dac013d239437829bc52dc70689c4ba15dc58 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionTest.java cc5e7d5256741ad0a48ff87c7f989a18b90f7f03 Diff: https://reviews.apache.org/r/59811/diff/3/ Changes: https://reviews.apache.org/r/59811/diff/2-3/ Testing (updated) --- 6/7/17: re-started precheckin precheckin is green with the exception of unrelated DUnit tests * re-starting precheckin once more Precheckin started Thanks, Ken Howe
Re: Review Request 59811: GEODE-2420: add file-size-limit param to the ExportLogsController
> On June 7, 2017, 9:40 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 122 (original), 123 (patched) > > <https://reviews.apache.org/r/59811/diff/2/?file=1743972#file1743972line123> > > > > I really like the "fail fast" idea! Can I suggest a minor reordering > > of a few lines that might be more readable? (The basic idea is just to > > move the lines into a narrower scope when possible). > > > > ``` > > List results = (List) estimateLogSize(args, > > server).getResult(); > > if (!results.isEmpty()) { > > if (results.get(0) instanceof Long) { > > long estimatedSize = (Long) results.get(0); > > logger.info("Received estimated export size from member > > {}: {}", server.getId(), > > estimatedSize); > > totalEstimatedExportSize += estimatedSize; > > } else if (results.get(0) instanceof ManagementException) { > > ManagementException exception = (ManagementException) > > results.get(0); > > return > > ResultBuilder.createUserErrorResult(exception.getMessage()); > > } > > } > > ``` Good suggestion, I do think it helps readability. - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59811/#review177230 --- On June 7, 2017, 10:23 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59811/ > --- > > (Updated June 7, 2017, 10:23 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2420: add file-size-limit param to the ExportLogsController > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 0ff780cbf66937d8ececfb3a2d0789ee485b9b62 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 57355c0efae4c6da9470267f95e27e59aa4d8b2c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce > > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java > a369c6e1ffb330715fbde2cd69d023ed36f133ad > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > 16549e70bbebf4390bb73a481274e92ca6cad035 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 8609b3aaf0a0eb1ba903bd39c64103f9510a6a78 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > 09ee08dd6af29b9a418ef7499defc4980da787ed > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsStatsDUnitTest.java > 44a036298e0991c880fc552596d296e104b97ca1 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 4e1dac013d239437829bc52dc70689c4ba15dc58 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionTest.java > cc5e7d5256741ad0a48ff87c7f989a18b90f7f03 > > > Diff: https://reviews.apache.org/r/59811/diff/2/ > > > Testing > --- > > 6/7/17: re-started precheckin > precheckin is green with the exception of unrelated DUnit tests > > Precheckin started > > > Thanks, > > Ken Howe > >
Re: Review Request 59811: GEODE-2420: add file-size-limit param to the ExportLogsController
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59811/ --- (Updated June 7, 2017, 6:35 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Revised the semantics of the export logs size warning: Previous: - file-size-limit option was for the size of the zip file that is exported to the locator. To make this check, all the work of exporting the logs was completed even thought the exported zipfile is eventually deleted if it was too big. - file-size-limit > 0 also enabled checking for potential disk overflow on each member as well as on the locator. This check use the estimated disk space required to filter and consolidate the fully expanded individual log files. - file-size-limit = 0 disables all checking of exported logs size and potential disk overflows Revised: - file-size-limit option will check that the estimated size of all of the fully expanded log files as exported to the locator is less than the specified limit (or the default of 100m if not specified). The work (and temporary disk space) of preparing and exporting the logs from all the members is not performed if the check fails (expnaded size on the locator is too big). - file-size-limit = 0 - no changes Refactored SizeExportLogsFunction. - Simplified the Result returned by the function. The result will always be a single Long (the estimated expnaded size of the filtered logs for the member) or an error result. Fixed the issue with file-size-limit not being recognized when connected to the locator via http Repository: geode Description --- GEODE-2420: add file-size-limit param to the ExportLogsController Diffs (updated) - geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 0ff780cbf66937d8ececfb3a2d0789ee485b9b62 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 57355c0efae4c6da9470267f95e27e59aa4d8b2c geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 9f68d3a5eaadbe8f1bd95ec8df85ed1f65aa67ce geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java a369c6e1ffb330715fbde2cd69d023ed36f133ad geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java 16549e70bbebf4390bb73a481274e92ca6cad035 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 8609b3aaf0a0eb1ba903bd39c64103f9510a6a78 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java 09ee08dd6af29b9a418ef7499defc4980da787ed geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsStatsDUnitTest.java 44a036298e0991c880fc552596d296e104b97ca1 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 4e1dac013d239437829bc52dc70689c4ba15dc58 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionTest.java cc5e7d5256741ad0a48ff87c7f989a18b90f7f03 Diff: https://reviews.apache.org/r/59811/diff/2/ Changes: https://reviews.apache.org/r/59811/diff/1-2/ Testing (updated) --- 6/7/17: re-started precheckin Precheckin started Thanks, Ken Howe
Review Request 59811: GEODE-2420: add file-size-limit param to the ExportLogsController
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59811/ --- Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, Patrick Rhomberg, and Swapnil Bawaskar. Repository: geode Description --- GEODE-2420: add file-size-limit param to the ExportLogsController Diffs - geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 0ff780cbf66937d8ececfb3a2d0789ee485b9b62 geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java a369c6e1ffb330715fbde2cd69d023ed36f133ad geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java 16549e70bbebf4390bb73a481274e92ca6cad035 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 8609b3aaf0a0eb1ba903bd39c64103f9510a6a78 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsStatsDUnitTest.java 44a036298e0991c880fc552596d296e104b97ca1 Diff: https://reviews.apache.org/r/59811/diff/1/ Testing --- Precheckin started Thanks, Ken Howe
Re: Review Request 59692: GEODE-2925: add target for resource operation for finer grained security
> On June 1, 2017, 5:09 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java > > Lines 228 (patched) > > <https://reviews.apache.org/r/59692/diff/3/?file=1737978#file1737978line228> > > > > I think it might be nice to have a variant of `authorize()` that takes > > a Resource/Operation/Target rather than their String representations: > > > > ``` > > public void authorize(Resource resource, Operation operation){} > > public void authorize(Resource resource, Operation operation, Target > > target){} > > > > ``` > > > > Then these methods would look like > > ``` > > public void authorizeDiskManage() { > > authorize(Resource.CLUSTER, Operation.MANAGE, > > ResourcePermission.Target.DISK); > > } > > ``` Target can be a region name as well as the as a Target enum. Consequently, the ResourcePermission constructors that the authorize methods call currently all expect target as a string. - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59692/#review176626 --- On June 1, 2017, 5:21 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59692/ > ------- > > (Updated June 1, 2017, 5:21 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2925: add target for resource operation for finer grained security > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java > 600d5462b1d18cfc702d400f6d91c1ac1fab3755 > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java > 14784c391212095413c0d577cfc65de7247080b5 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java > 226cfaf45fa2a1720a92e8e7ac2c179653240e2d > > geode-core/src/main/java/org/apache/geode/management/internal/security/MBeanServerWrapper.java > fe79efbed0aa7ec9a3d27526df2f4a86794513c2 > > geode-core/src/main/java/org/apache/geode/management/internal/security/ResourceOperation.java > db3a1872a87b558772902cf14580f3e14fca97b3 > geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java > 45da464419779773c9116d824fcf11774bafbd79 > > geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java > b728b271efb876d471b35e002c5b110919f10fcc > > geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java > 3f8f4d9d4ee0a8f9c3385cd66ee20655d126d34d > > geode-core/src/test/resources/org/apache/geode/codeAnalysis/excludedClasses.txt > 9cff80d1982bd30f6ba5d8a61ab7307a69862fd4 > > > Diff: https://reviews.apache.org/r/59692/diff/4/ > > > Testing > --- > > precheckin runing > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59643: GEODE-3006: reduce the frequency of ping request and reduce the loglevel of login/logout messages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59643/#review176546 --- Ship it! Ship It! - Ken Howe On May 31, 2017, 10:46 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59643/ > --- > > (Updated May 31, 2017, 10:46 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > disregard the diff for log4j2.xml changes, these are from the changeset of > another review request. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java > 600d5462b1d18cfc702d400f6d91c1ac1fab3755 > > geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java > b342c6688e527b0ed67e0210c6f8befa15978818 > > > Diff: https://reviews.apache.org/r/59643/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.
> On May 31, 2017, 10:18 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java > > Lines 48 (patched) > > <https://reviews.apache.org/r/59686/diff/1/?file=1735898#file1735898line48> > > > > I worry that a user may at some point specify a `--J` option that > > includes `,,`. I think our code could be more robust by using a delimeter > > that a user can't type on a standard keyboard. The ASCII "Unit separator" > > character (decimal code 31, hex 0x1f) seems like a natural candidate. That > > would make this look like: > > > > ``` > > private static final char ASCII_UNIT_SEPARATOR = '\u001F'; > > public static final String JARGUMENT_SPLIREGX = "" + > > ASCII_UNIT_SEPARATOR; > > ``` > > > > > > I also think that `J_ARGUMENT_DELIMITER` might be a more informative > > name. +1 - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59686/#review176535 --- On May 31, 2017, 5:42 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59686/ > ------- > > (Updated May 31, 2017, 5:42 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2983: correctly handling --J option value that has "," inside. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java > 288ea054ae1230c480d141c0159d6ccf9c299a7d > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java > 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java > 4467792f60a2a3bf7cc53cf35e997e7462882539 > > > Diff: https://reviews.apache.org/r/59686/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59692: GEODE-2925: add target for resource operation for finer grained security
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59692/#review176533 --- geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java Line 77 (original), 95 (patched) <https://reviews.apache.org/r/59692/#comment249906> I think it would be better to use Region.SEPARATOR instead of "/". - Ken Howe On May 31, 2017, 8:55 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59692/ > --- > > (Updated May 31, 2017, 8:55 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2925: add target for resource operation for finer grained security > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java > 600d5462b1d18cfc702d400f6d91c1ac1fab3755 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java > 226cfaf45fa2a1720a92e8e7ac2c179653240e2d > > geode-core/src/main/java/org/apache/geode/management/internal/security/ResourceOperation.java > db3a1872a87b558772902cf14580f3e14fca97b3 > geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java > 45da464419779773c9116d824fcf11774bafbd79 > > geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java > b728b271efb876d471b35e002c5b110919f10fcc > > geode-core/src/test/java/org/apache/geode/management/internal/security/TestCommand.java > 3f8f4d9d4ee0a8f9c3385cd66ee20655d126d34d > > > Diff: https://reviews.apache.org/r/59692/diff/1/ > > > Testing > --- > > precheckin runing > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59611: GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59611/#review176383 --- geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java Lines 171 (patched) <https://reviews.apache.org/r/59611/#comment249760> Remove extra blank line geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java Line 63 (original), 41 (patched) <https://reviews.apache.org/r/59611/#comment249761> This new method in a product class is only used in a test class. It would be better to move this out of product code to the test where it's needed. - Ken Howe On May 26, 2017, 10:02 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59611/ > --- > > (Updated May 26, 2017, 10:02 p.m.) > > > Review request for geode, Emily Yeh, Jinmei Liao, and Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2989: Improve mechanism for scanning the classpath to find gfsh commands > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/CommandManager.java > 0576e46fce08f9c969726817e0012a2094f79fbe > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/ClasspathScanLoadHelper.java > 20fffbd5c492cfb4642ce41c937da3d499d3434c > > geode-core/src/test/java/org/apache/geode/management/internal/cli/ClasspathScanLoadHelperJUnitTest.java > a13ca351c49da2bc523e6d3ad9dd3e845b7b0429 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshInitFileJUnitTest.java > 159c47ffbd71c6d08b563d8d28d5d7cdc4fb096b > > geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java > 65fd528641771e535f3d8d0d6601cef53f91af7a > > geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java > e9523862da9e045b05417dd8123574b01622c497 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > 30ae59fd786b4753ae71849f81deeb0fe7f74c17 > > geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/ShellCommandsControllerJUnitTest.java > 10e26f6c5d006856e9e88b06a60f5e67cb68a2ce > > > Diff: https://reviews.apache.org/r/59611/diff/1/ > > > Testing > --- > > - Precheckin passed > - Further cleanup of CommandManager is expected in a subsequent ticket > > > Thanks, > > Jared Stewart > >
Re: Review Request 59642: GEODE-3000: do not have jetty log at debug level
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59642/#review176378 --- Ship it! Ship It! - Ken Howe On May 30, 2017, 6:07 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59642/ > --- > > (Updated May 30, 2017, 6:07 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-3000: do not have jetty log at debug level > > > Diffs > - > > geode-core/src/main/resources/log4j2.xml > cdb932b5b3b3337aae4a6cde9800ebd5617a0c4d > > geode-core/src/test/java/org/apache/geode/management/internal/security/LogNoPasswordTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59642/diff/1/ > > > Testing > --- > > manual tests and precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59653: GEODE-2420 Warn a user if they try to export too much data. Added gfsh ref docs for --file-size-limit option.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59653/#review176362 --- Ship it! Ship It! - Ken Howe On May 30, 2017, 9:21 p.m., Dave Barnes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59653/ > --- > > (Updated May 30, 2017, 9:21 p.m.) > > > Review request for geode, Joey McAllister, Karen Miller, and Ken Howe. > > > Repository: geode > > > Description > --- > > GEODE-2420 Warn a user if they try to export too much data. Added gfsh ref > docs for --file-size-limit option. > > > Diffs > - > > geode-docs/tools_modules/gfsh/command-pages/export.html.md.erb > ccf95ccf2d8449afeb1dc8381b1c0b5a05c36796 > > > Diff: https://reviews.apache.org/r/59653/diff/1/ > > > Testing > --- > > > Thanks, > > Dave Barnes > >
Re: Review Request 59542: GEODE-2974: rename ResultBuilder methods: GemFire -> Geode
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59542/ --- (Updated May 30, 2017, 5:54 p.m.) Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Reduces scope of the change to ResultBuilder class; the original method names have been restored. Error message text removes mention of "GemFire" or "Geode" to now report: "Could not process command due to error." plus the message passed to the ResultBuilder Repository: geode Description --- Change GemFire to Geode in method names and message string This is change to simply correct the Gfsh command error strings where "GemFire" appears. With additional work on Gfsh command error handling pending (see GEODE-2984), no attempt has been made within the scope of this issue to reorganize or otherwise improve Gfsh command error messages. Diffs (updated) - geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java 6332540aa4ff6d210ed7f3ba167057de9a0a5023 Diff: https://reviews.apache.org/r/59542/diff/2/ Changes: https://reviews.apache.org/r/59542/diff/1-2/ Testing (updated) --- 5/30 - precheckin restarted precheckin is running Thanks, Ken Howe
Review Request 59542: GEODE-2974: rename ResultBuilder methods: GemFire -> Geode
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59542/ --- Review request for geode, Emily Yeh, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Repository: geode Description --- Change GemFire to Geode in method names and message string This is change to simply correct the Gfsh command error strings where "GemFire" appears. With additional work on Gfsh command error handling pending (see GEODE-2984), no attempt has been made within the scope of this issue to reorganize or otherwise improve Gfsh command error messages. Diffs - geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java 18940fc0c6c0f36a7dd5cab5723d426d80df3cb9 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ConfigCommands.java 6d3f50ff2bcdf9d0450dd40a8defb83c2f2f6e04 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateAlterDestroyRegionCommands.java b8ebc4917d88ce089ec2a19d606e004c1e5c6b33 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java a38e54504e949905f222d9891f624e373219364b geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java 4018beb8dfbd4bb639aec559991452226f15c908 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DiskStoreCommands.java 4232d91edc9a74db144d49542d79f7cfd9f4a98b geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DurableClientCommands.java bcbfcf0ca72876dbfb3d41613159aa051726870e geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportImportClusterConfigurationCommands.java ea101826493bfbc9211b4c673c596d7514a7f431 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java b824bc915d453defe1c665eb7811a782f2c72fdf geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsInterceptor.java 5f2184889d28f441eb639d7ddf4ec2b7abfa293b geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java 2007e4af516274ac815cc3b06b9928f1aaed313f geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/IndexCommands.java a4ba64c608f184fb130eb5ca3a05822f6cf82818 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MemberCommands.java 415dd691cc6ed14bc67626881eecf65d5e446c99 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/MiscellaneousCommands.java 0d714f4c295e2aded0c0fd28bbf4f72ffcdd14a7 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXCommands.java 0ce8ec272eee9f07a05aa0ac478e136163ee41e6 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/QueueCommands.java 6208adb63cc29f213c035849c78c3809f61b3f16 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/RegionCommands.java 561d4b8514c5b4500bf870e0b9d0ef3bcdbaf40d geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShellCommands.java ad344ff598db5474846d1b6b334d377e01c5fbd7 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/WanCommands.java feeb3533e84fafbf22e5f9283690fe2ef0edf27e geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java fe88fc98d397fa748f8f54b900b1511eb114c0b6 geode-core/src/main/java/org/apache/geode/management/internal/cli/remote/CommandProcessor.java c2c6e1425d71af9d2ea59046b17afd70ad30dd68 geode-core/src/main/java/org/apache/geode/management/internal/cli/result/ResultBuilder.java 6332540aa4ff6d210ed7f3ba167057de9a0a5023 geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java 2b39bedd1bcceef50e3f4933e17581048142a29b geode-lucene/src/main/java/org/apache/geode/cache/lucene/internal/cli/LuceneIndexCommands.java 5e17f6e55e6726f22cc39ddaba1ec608ca5cab9d Diff: https://reviews.apache.org/r/59542/diff/1/ Testing --- precheckin is running Thanks, Ken Howe
Re: Review Request 59505: GEODE-2964: add common-collections to gfsh dependencies
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59505/#review175871 --- Ship it! Ship It! - Ken Howe On May 23, 2017, 10:11 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59505/ > --- > > (Updated May 23, 2017, 10:11 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2964: add common-collections to gfsh dependencies > > > Diffs > - > > geode-assembly/build.gradle a4f0c69ee39342d9a3350d8e3a32cd65b560c75a > > > Diff: https://reviews.apache.org/r/59505/diff/1/ > > > Testing > --- > > gfsh manual tests > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/ --- (Updated May 23, 2017, 3:45 p.m.) Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- OK, ready to review now. Sorted out the repo sync and manually fixed the renames in the tests. The correct changes to review are between revisions 3 & 5. Ignore revision 4. Repository: geode Description --- Adds 'export logs' option, --file-limit-size, to allow user to set maximun size of the epxorted logs zip file. When size checking is enabled (file-limit-size > 0) then the check will also prevent filling up the disk on each member while consolidating and filtering the logs. Diffs (updated) - geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java 7170f209ffa169fb6efdc851d35b61a2031888b7 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java 663a08e15624ed3dbc032460133fe3c62fc5ac26 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java c175e1ae3def869890692461bd129891350b383c geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 8d20dc05c14bf558462893c4dd4cbbc474df4077 geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 68d055cbd61ca35ef7409ff3370214a005da3d9b geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java 0a799f6c85dada2791da57585234fa2e47ef0b3d geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java a02c07f2c28156e097306f4b57174cddeda78845 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 96ac76588662b1de5d5bf41c24ab115d90fc0a85 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java ec2bcfe8ea876172c6946c43c005659d23d055e0 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 90a92f33247ecec8ee300ecb80a5d8ab27193c94 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java d8f2f2db937fc51ab5f917659e766f338b9ae847 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java e70a750f48a8f7cc3b10b89be9b5934944addb0d geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java a387af3b70f61256b5d9303de29e9402bbdd71e6 geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogSizerTest.java c7b3ab934ce1cf97c0a70bc23b50f0f5ca08bb40 Diff: https://reviews.apache.org/r/59287/diff/5/ Changes: https://reviews.apache.org/r/59287/diff/4-5/ Testing --- Precheckin is in progress - all green so far with only DistributedTest still running 5/22 - Precheckin being re-run. Currently Green expcet for the known LocatorLauncher test failures. DistributedTests still running 5/23 - Re-running precheckin after syncing with current state of develop Thanks, Ken Howe
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/#review175794 --- Hold off on reviewing this update! The IDE didn't handle the renames correctly, and the diff from my repo ended up with some extra changes. - Ken Howe On May 23, 2017, 2:52 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59287/ > --- > > (Updated May 23, 2017, 2:52 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Adds 'export logs' option, --file-limit-size, to allow user to set > maximun size of the epxorted logs zip file. > > When size checking is enabled (file-limit-size > 0) then the check > will also prevent filling up the disk on each member while consolidating > and filtering the logs. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java > 13a1721ae3967f6503f7a1042b4dddf246b83645 > > geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java > 7caad3f33ee55f72dc61c4adc2ab3de5429a1607 > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > 7170f209ffa169fb6efdc851d35b61a2031888b7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > 663a08e15624ed3dbc032460133fe3c62fc5ac26 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java > c175e1ae3def869890692461bd129891350b383c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 8d20dc05c14bf558462893c4dd4cbbc474df4077 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 68d055cbd61ca35ef7409ff3370214a005da3d9b > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java > 0a799f6c85dada2791da57585234fa2e47ef0b3d > > geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java > d63da8755bed1041296d40ea9821251f01c4c59f > > geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java > 4043888561d4206b1c0b405b3b77d5c9876ad99a > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > a02c07f2c28156e097306f4b57174cddeda78845 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 96ac76588662b1de5d5bf41c24ab115d90fc0a85 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > ec2bcfe8ea876172c6946c43c005659d23d055e0 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 90a92f33247ecec8ee300ecb80a5d8ab27193c94 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java > 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java > d8f2f2db937fc51ab5f917659e766f338b9ae847 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java > e70a750f48a8f7cc3b10b89be9b5934944addb0d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java > a387af3b70f61256b5d9303de29e9402bbdd71e6 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogSizerTest.java > c7b3ab934ce1cf97c0a70bc23b50f0f5ca08bb40 > > > Diff: https://reviews.apache.org/r/59287/diff/4/ > > > Testing > --- > > Precheckin is in progress - all green so far with only DistributedTest still > running > > 5/22 - Precheckin being re-run. Currently Green expcet for the known > LocatorLauncher test failures. DistributedTests still running > > 5/23 - Re-running precheckin after syncing with current state of develop > > > Thanks, > > Ken Howe > >
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/ --- (Updated May 23, 2017, 2:52 p.m.) Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- renamed methods and updated javadocs Repository: geode Description --- Adds 'export logs' option, --file-limit-size, to allow user to set maximun size of the epxorted logs zip file. When size checking is enabled (file-limit-size > 0) then the check will also prevent filling up the disk on each member while consolidating and filtering the logs. Diffs (updated) - geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java 13a1721ae3967f6503f7a1042b4dddf246b83645 geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java 7caad3f33ee55f72dc61c4adc2ab3de5429a1607 geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java 7170f209ffa169fb6efdc851d35b61a2031888b7 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java 663a08e15624ed3dbc032460133fe3c62fc5ac26 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java c175e1ae3def869890692461bd129891350b383c geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 8d20dc05c14bf558462893c4dd4cbbc474df4077 geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 68d055cbd61ca35ef7409ff3370214a005da3d9b geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java 0a799f6c85dada2791da57585234fa2e47ef0b3d geode-core/src/test/java/org/apache/geode/distributed/LocatorLauncherTest.java d63da8755bed1041296d40ea9821251f01c4c59f geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java 4043888561d4206b1c0b405b3b77d5c9876ad99a geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java a02c07f2c28156e097306f4b57174cddeda78845 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 96ac76588662b1de5d5bf41c24ab115d90fc0a85 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java ec2bcfe8ea876172c6946c43c005659d23d055e0 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 90a92f33247ecec8ee300ecb80a5d8ab27193c94 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java d8f2f2db937fc51ab5f917659e766f338b9ae847 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java e70a750f48a8f7cc3b10b89be9b5934944addb0d geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java a387af3b70f61256b5d9303de29e9402bbdd71e6 geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogSizerTest.java c7b3ab934ce1cf97c0a70bc23b50f0f5ca08bb40 Diff: https://reviews.apache.org/r/59287/diff/4/ Changes: https://reviews.apache.org/r/59287/diff/3-4/ Testing (updated) --- Precheckin is in progress - all green so far with only DistributedTest still running 5/22 - Precheckin being re-run. Currently Green expcet for the known LocatorLauncher test failures. DistributedTests still running 5/23 - Re-running precheckin after syncing with current state of develop Thanks, Ken Howe
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
> On May 22, 2017, 10:08 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 300 (original), 268 (patched) > > <https://reviews.apache.org/r/59287/diff/2-3/?file=1720360#file1720360line301> > > > > If I just saw the name of this method, I would expect it to return a > > boolean. Perhaps a name like `void throwIfFileSizeExceedsLimit` would make > > the intent more explicit. Forgot to rename this when I changed the method type from boolean to void. Renamed similar to what Kirk suggested, checkFileSizeWithinLimit. > On May 22, 2017, 10:08 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 319 (original), 286 (patched) > > <https://reviews.apache.org/r/59287/diff/2-3/?file=1720360#file1720360line324> > > > > As above, perhaps something like `void > > throwIfSizeExceedsDiskSpaceOfMember` would better indicate the intent of > > this method. > > Kirk Lund wrote: > It's also common to name the method something like "checkFileSizeLimit" Renamed to checkIfExportLogsOverflowsDisk. I updated the javadocs to explicitly call out the exception that's thrown. - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/#review175719 --- On May 22, 2017, 6:14 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59287/ > --- > > (Updated May 22, 2017, 6:14 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Adds 'export logs' option, --file-limit-size, to allow user to set > maximun size of the epxorted logs zip file. > > When size checking is enabled (file-limit-size > 0) then the check > will also prevent filling up the disk on each member while consolidating > and filtering the logs. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > 7170f209ffa169fb6efdc851d35b61a2031888b7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > 663a08e15624ed3dbc032460133fe3c62fc5ac26 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java > c175e1ae3def869890692461bd129891350b383c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 8d20dc05c14bf558462893c4dd4cbbc474df4077 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 68d055cbd61ca35ef7409ff3370214a005da3d9b > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java > 0a799f6c85dada2791da57585234fa2e47ef0b3d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > a02c07f2c28156e097306f4b57174cddeda78845 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 96ac76588662b1de5d5bf41c24ab115d90fc0a85 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > ec2bcfe8ea876172c6946c43c005659d23d055e0 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 90a92f33247ecec8ee300ecb80a5d8ab27193c94 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java > 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java > d8f2f2db937fc51ab5f917659e766f338b9ae847 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java > e70a750f48a8f7cc3b10b89be9b5934944addb0d > > geode-core/src/test/java/org/apache/geode/management/internal/cli
Re: Review Request 59457: GEODE-2959: remove DistributedSystem instance in tearDown
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59457/#review175674 --- Ship it! Ship It! - Ken Howe On May 22, 2017, 6:22 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59457/ > --- > > (Updated May 22, 2017, 6:22 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick > Rhomberg. > > > Bugs: GEODE-2959 > https://issues.apache.org/jira/browse/GEODE-2959 > > > Repository: geode > > > Description > --- > > CacheXmlParserJUnitTest#testCacheXmlParserWithSimplePool uses > InternalDistributedSystem#newInstanceForTesting to add a mock > DistributedSystem to existingSystems. This then causes LocatorLauncherTest to > fail because that test expects existingSystems to contain zero instances. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java > 29ebf06724fa28de1abdf5174303dd1849e74164 > > geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java > eeb44e7a331c1f10f6a40dd39e8b9143671096ed > > geode-core/src/test/java/org/apache/geode/internal/cache/xmlcache/CacheXmlParserJUnitTest.java > b0f5c8dc39ff557e6a550bf17eb61fdfc594fd64 > > > Diff: https://reviews.apache.org/r/59457/diff/2/ > > > Testing > --- > > precheckin in progress > > > Thanks, > > Kirk Lund > >
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
> On May 16, 2017, 5:59 p.m., Jinmei Liao wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Lines 149 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720360#file1720360line154> > > > > looks like the locator decides if each server's exported log size > > exeedes the limit of each server, should we push this check onto the server > > itself? Changed to do the size check on each member. > On May 16, 2017, 5:59 p.m., Jinmei Liao wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Lines 167 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720360#file1720360line172> > > > > This seems like a hacky way to do testing. I usally hate to see test > > code mixed up in the production code. Removed the test hook, and moved a Unit test to and Integration test of SizeExportLogsFunction. > On May 16, 2017, 5:59 p.m., Jinmei Liao wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 241 (original), 319 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720360#file1720360line324> > > > > the returned boolean doesn't seem to be used. It looks like this > > function throws an exception if exeeding limit. See note from jstewart's review - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/#review175127 --- On May 22, 2017, 6:14 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59287/ > --- > > (Updated May 22, 2017, 6:14 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Adds 'export logs' option, --file-limit-size, to allow user to set > maximun size of the epxorted logs zip file. > > When size checking is enabled (file-limit-size > 0) then the check > will also prevent filling up the disk on each member while consolidating > and filtering the logs. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > 7170f209ffa169fb6efdc851d35b61a2031888b7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > 663a08e15624ed3dbc032460133fe3c62fc5ac26 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java > c175e1ae3def869890692461bd129891350b383c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 8d20dc05c14bf558462893c4dd4cbbc474df4077 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 68d055cbd61ca35ef7409ff3370214a005da3d9b > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java > 0a799f6c85dada2791da57585234fa2e47ef0b3d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > a02c07f2c28156e097306f4b57174cddeda78845 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 96ac76588662b1de5d5bf41c24ab115d90fc0a85 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > ec2bcfe8ea876172c6946c43c005659d23d055e0 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 90a92f33247ecec8ee300ecb80a5d8ab27193c94 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java > 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java > d8f2f2db937fc51ab5f917659e766f338b9ae847 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeE
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
> On May 15, 2017, 8:53 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > > Lines 160 (patched) > > <https://reviews.apache.org/r/59287/diff/1/?file=1719248#file1719248line167> > > > > My merge to develop might cause you some weirdness. You might want to > > mock InternalCache instead but I'm not sure. Correct. Changed to use InternalCache everywhere in ExportLogsCommandTest. Also change the access of ExportLogsCommand.getCache to package-private so it can spied it in the tests. - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/#review175023 ------- On May 22, 2017, 6:14 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59287/ > --- > > (Updated May 22, 2017, 6:14 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Adds 'export logs' option, --file-limit-size, to allow user to set > maximun size of the epxorted logs zip file. > > When size checking is enabled (file-limit-size > 0) then the check > will also prevent filling up the disk on each member while consolidating > and filtering the logs. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > 7170f209ffa169fb6efdc851d35b61a2031888b7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > 663a08e15624ed3dbc032460133fe3c62fc5ac26 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java > c175e1ae3def869890692461bd129891350b383c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 8d20dc05c14bf558462893c4dd4cbbc474df4077 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 68d055cbd61ca35ef7409ff3370214a005da3d9b > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java > 0a799f6c85dada2791da57585234fa2e47ef0b3d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > a02c07f2c28156e097306f4b57174cddeda78845 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 96ac76588662b1de5d5bf41c24ab115d90fc0a85 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > ec2bcfe8ea876172c6946c43c005659d23d055e0 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 90a92f33247ecec8ee300ecb80a5d8ab27193c94 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java > 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java > d8f2f2db937fc51ab5f917659e766f338b9ae847 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java > e70a750f48a8f7cc3b10b89be9b5934944addb0d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java > a387af3b70f61256b5d9303de29e9402bbdd71e6 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogSizerTest.java > c7b3ab934ce1cf97c0a70bc23b50f0f5ca08bb40 > > > Diff: https://reviews.apache.org/r/59287/diff/3/ > > > Testing > --- > > Precheckin is in progress - all green so far with only DistributedTest still > running > > 5/22 - Precheckin being re-run. Currently Green expcet for the known > LocatorLauncher test failures. DistributedTests still running > > > Thanks, > > Ken Howe > >
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
> On May 16, 2017, 5:19 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 230 (original), 300 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720360#file1720360line305> > > > > It doesn't look like the return values of this method or the method on > > 319 are ever used in the product code. Does it make sense to have these > > return a boolean rather than void? Reverted this method to a void type and refactored tests. > On May 16, 2017, 5:19 p.m., Jared Stewart wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > > Line 241 (original), 319 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720360#file1720360line324> > > > > The `diskSize` parameter in this method looks to be unused. After refactoring to do the size check on on each member instead of back at the locator, the only item returned from SizeExportLogFunction is the estimated size for that member. Consequently, I've removed the ExportedLogsSizeInfo, and now just retiurn a single Long as the result (or error if the size check fails). - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/#review175123 ------- On May 22, 2017, 6:14 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59287/ > --- > > (Updated May 22, 2017, 6:14 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Adds 'export logs' option, --file-limit-size, to allow user to set > maximun size of the epxorted logs zip file. > > When size checking is enabled (file-limit-size > 0) then the check > will also prevent filling up the disk on each member while consolidating > and filtering the logs. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > 7170f209ffa169fb6efdc851d35b61a2031888b7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > 663a08e15624ed3dbc032460133fe3c62fc5ac26 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java > c175e1ae3def869890692461bd129891350b383c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 8d20dc05c14bf558462893c4dd4cbbc474df4077 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java > 68d055cbd61ca35ef7409ff3370214a005da3d9b > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java > a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java > 0a799f6c85dada2791da57585234fa2e47ef0b3d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java > a02c07f2c28156e097306f4b57174cddeda78845 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java > 96ac76588662b1de5d5bf41c24ab115d90fc0a85 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java > ec2bcfe8ea876172c6946c43c005659d23d055e0 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java > 90a92f33247ecec8ee300ecb80a5d8ab27193c94 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java > 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java > d8f2f2db937fc51ab5f917659e766f338b9ae847 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java > e70a750f48a8f7cc3b10b89be9b5934944addb0d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java > a387af3b70f61256b5d9303de29e9402bbdd71e6 > > geode-co
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
> On May 17, 2017, 5:38 p.m., Patrick Rhomberg wrote: > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > > Line 28 (original), 28 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720359#file1720359line28> > > > > Are star-imports frowned upon? IntelliJ optimizes imports to... > > > > ``` > > import org.apache.geode.DataSerializer; > > import org.apache.geode.GemFireConfigException; > > import org.apache.geode.InternalGemFireError; > > import org.apache.geode.cache.UnsupportedVersionException; > > import org.apache.geode.distributed.DistributedMember; > > import org.apache.geode.distributed.DurableClientAttributes; > > import org.apache.geode.distributed.Role; > > import > > org.apache.geode.distributed.internal.DistributionAdvisor.ProfileId; > > import org.apache.geode.distributed.internal.DistributionConfig; > > import org.apache.geode.distributed.internal.DistributionManager; > > import org.apache.geode.distributed.internal.ServerLocation; > > import org.apache.geode.internal.Assert; > > import org.apache.geode.internal.DataSerializableFixedID; > > import org.apache.geode.internal.InternalDataSerializer; > > import org.apache.geode.internal.OSProcess; > > import org.apache.geode.internal.Version; > > import org.apache.geode.internal.cache.versions.VersionSource; > > import org.apache.geode.internal.i18n.LocalizedStrings; > > import org.apache.geode.internal.net.SocketCreator; > > > > import java.io.DataInput; > > import java.io.DataOutput; > > import java.io.EOFException; > > import java.io.Externalizable; > > import java.io.IOException; > > import java.io.ObjectInput; > > import java.io.ObjectOutput; > > import java.net.InetAddress; > > import java.net.UnknownHostException; > > import java.util.Arrays; > > import java.util.Collections; > > import java.util.HashSet; > > import java.util.List; > > import java.util.Set; > > ``` Did not make this change. The only changes in this file that I made were removing contributor's name from comments that I noticed while referencing the file during test development. Changes like this should be made on a separate ticket. > On May 17, 2017, 5:38 p.m., Patrick Rhomberg wrote: > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > > Line 1208 (original), 1208 (patched) > > <https://reviews.apache.org/r/59287/diff/2/?file=1720359#file1720359line1208> > > > > InternalDistrubtedMemberWrapper -> InternalDistributedMemberWrapper See reply above regarding changes to InternalDistributedMember - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/#review175282 --- On May 22, 2017, 6:14 p.m., Ken Howe wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59287/ > --- > > (Updated May 22, 2017, 6:14 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > Adds 'export logs' option, --file-limit-size, to allow user to set > maximun size of the epxorted logs zip file. > > When size checking is enabled (file-limit-size > 0) then the check > will also prevent filling up the disk on each member while consolidating > and filtering the logs. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java > 7170f209ffa169fb6efdc851d35b61a2031888b7 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java > 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java > 663a08e15624ed3dbc032460133fe3c62fc5ac26 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java > c175e1ae3def869890692461bd129891350b383c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java > 8d20dc05c14bf558462893c4dd4cbbc474df4
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/ --- (Updated May 22, 2017, 6:14 p.m.) Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Changes --- Updats from reviews. Repository: geode Description --- Adds 'export logs' option, --file-limit-size, to allow user to set maximun size of the epxorted logs zip file. When size checking is enabled (file-limit-size > 0) then the check will also prevent filling up the disk on each member while consolidating and filtering the logs. Diffs (updated) - geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java 7170f209ffa169fb6efdc851d35b61a2031888b7 geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunction.java 663a08e15624ed3dbc032460133fe3c62fc5ac26 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfo.java c175e1ae3def869890692461bd129891350b383c geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 8d20dc05c14bf558462893c4dd4cbbc474df4077 geode-core/src/main/java/org/apache/geode/management/internal/cli/i18n/CliStrings.java 68d055cbd61ca35ef7409ff3370214a005da3d9b geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java a0be7fbd83918fccb5254b4a48ba7bf14a0fb344 geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java 0a799f6c85dada2791da57585234fa2e47ef0b3d geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java a02c07f2c28156e097306f4b57174cddeda78845 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 96ac76588662b1de5d5bf41c24ab115d90fc0a85 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java ec2bcfe8ea876172c6946c43c005659d23d055e0 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 90a92f33247ecec8ee300ecb80a5d8ab27193c94 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportedLogsSizeInfoTest.java 0bfbefa90af7813a8cf20529d36c9cbe5111f9d9 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java d8f2f2db937fc51ab5f917659e766f338b9ae847 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsTestSuite.java e70a750f48a8f7cc3b10b89be9b5934944addb0d geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterTest.java a387af3b70f61256b5d9303de29e9402bbdd71e6 geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogSizerTest.java c7b3ab934ce1cf97c0a70bc23b50f0f5ca08bb40 Diff: https://reviews.apache.org/r/59287/diff/3/ Changes: https://reviews.apache.org/r/59287/diff/2-3/ Testing (updated) --- Precheckin is in progress - all green so far with only DistributedTest still running 5/22 - Precheckin being re-run. Currently Green expcet for the known LocatorLauncher test failures. DistributedTests still running Thanks, Ken Howe
Re: Review Request 59384: GEODE-1930: temporarily disable verifySystemNotifications
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59384/#review175566 --- Ship it! Ship It! - Ken Howe On May 19, 2017, 5:06 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59384/ > --- > > (Updated May 19, 2017, 5:06 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick > Rhomberg. > > > Bugs: GEODE-1930 > https://issues.apache.org/jira/browse/GEODE-1930 > > > Repository: geode > > > Description > --- > > I'm consistently seeing RegionManagementDUnitTest.testRegionAggregate fail in > verifySystemNotifications which is at the very end of the test. I think there > must be something async going on that we need to find and Await on. > > Until one of us has time to fix verifySystemNotifications, I'd like to > comment out that one line so we aren't seeing this fail every precheckin. > > I used GEODE-1930 which is a ticket to refactor the Management DUnit tests > because it's still open until we can finish refactoring the last couple > Management DUnit tests which includes RegionManagementDUnitTest. > > > Diffs > - > > > geode-core/src/test/java/org/apache/geode/management/RegionManagementDUnitTest.java > 7dabd61d51f8598640537b661ecbb076b212cd0d > > > Diff: https://reviews.apache.org/r/59384/diff/1/ > > > Testing > --- > > precheckin passed > > > Thanks, > > Kirk Lund > >
Re: Review Request 59299: GEODE-2874: Fix StringIndexOutOfBoundsException while initializing logger
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59299/#review175041 --- Ship it! Ship It! - Ken Howe On May 15, 2017, 10:31 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59299/ > --- > > (Updated May 15, 2017, 10:31 p.m.) > > > Review request for geode. > > > Repository: geode > > > Description > --- > > GEODE-2874: Fix StringIndexOutOfBoundsException while initializing logger > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandler.java > f814aeb > > geode-core/src/test/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandlerDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandlerIntegrationTest.java > 5ea77c4 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > b86e058 > > > Diff: https://reviews.apache.org/r/59299/diff/2/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 59299: GEODE-2874: Fix StringIndexOutOfBoundsException while initializing logger
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59299/#review175036 --- geode-core/src/test/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandlerDUnitTest.java Lines 1 (patched) <https://reviews.apache.org/r/59299/#comment248353> Formatting of the license note geode-core/src/test/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandlerDUnitTest.java Lines 42 (patched) <https://reviews.apache.org/r/59299/#comment248354> I'd favor a more descriptive test name that won't prompt digging through a JIRA to figure out what the intent is. "testGeode2874_nameWithoutExtensionDoesntThrow"? - Ken Howe On May 15, 2017, 10:04 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59299/ > --- > > (Updated May 15, 2017, 10:04 p.m.) > > > Review request for geode. > > > Repository: geode > > > Description > --- > > GEODE-2874: Fix StringIndexOutOfBoundsException while initializing logger > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandler.java > f814aebb8717c3a53c72d4cdf55b81d72e0fbed5 > > geode-core/src/test/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandlerDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/io/MainWithChildrenRollingFileHandlerIntegrationTest.java > 5ea77c4119b7551d0f2fd8283f5ce6108b3295a1 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > b86e058960e42e4d276f8fd66c26bb3fc4d3d876 > > > Diff: https://reviews.apache.org/r/59299/diff/1/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/ --- (Updated May 15, 2017, 10:09 p.m.) Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Repository: geode Description --- Adds 'export logs' option, --file-limit-size, to allow user to set maximun size of the epxorted logs zip file. When size checking is enabled (file-limit-size > 0) then the check will also prevent filling up the disk on each member while consolidating and filtering the logs. Diffs (updated) - geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java 41c85d6421c8283163b70f2a560c8e4cbb02f2cc geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 20ec1f5702aea341ace5aa3b103c34cbdce1ae87 geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 8d20dc05c14bf558462893c4dd4cbbc474df4077 geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java 0a799f6c85dada2791da57585234fa2e47ef0b3d geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java a02c07f2c28156e097306f4b57174cddeda78845 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 96ac76588662b1de5d5bf41c24ab115d90fc0a85 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java ec2bcfe8ea876172c6946c43c005659d23d055e0 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 90a92f33247ecec8ee300ecb80a5d8ab27193c94 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java d8f2f2db937fc51ab5f917659e766f338b9ae847 Diff: https://reviews.apache.org/r/59287/diff/2/ Changes: https://reviews.apache.org/r/59287/diff/1-2/ Testing --- Precheckin is in progress - all green so far with only DistributedTest still running Thanks, Ken Howe
Review Request 59287: GEODE-2420: Enable export logs size estimation and user warning
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59287/ --- Review request for geode, Jinmei Liao, Jared Stewart, Kirk Lund, and Patrick Rhomberg. Repository: geode Description --- Adds 'export logs' option, --file-limit-size, to allow user to set maximun size of the epxorted logs zip file. When size checking is enabled (file-limit-size > 0) then the check will also prevent filling up the disk on each member while consolidating and filtering the logs. Diffs - geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java 41c85d6421c8283163b70f2a560c8e4cbb02f2cc geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommand.java 647fff2edd5cb7aae56ec994747354ad2adefd9a geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunction.java 06af662f21f2bcf9abec8fa25ff7ea6ddb38d1e4 geode-core/src/main/java/org/apache/geode/management/internal/cli/util/LogSizer.java 0a799f6c85dada2791da57585234fa2e47ef0b3d geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsCommandTest.java a02c07f2c28156e097306f4b57174cddeda78845 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsDUnitTest.java 96ac76588662b1de5d5bf41c24ab115d90fc0a85 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsFileSizeLimitTest.java ec2bcfe8ea876172c6946c43c005659d23d055e0 geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsTestSuite.java 90a92f33247ecec8ee300ecb80a5d8ab27193c94 geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/SizeExportLogsFunctionCacheTest.java d8f2f2db937fc51ab5f917659e766f338b9ae847 Diff: https://reviews.apache.org/r/59287/diff/1/ Testing --- Precheckin is in progress - all green so far with only DistributedTest still running Thanks, Ken Howe
Re: Review Request 59246: GEODE-2876: reset isGfshVM flag to false when tearing down tests using CliCommandTestBase.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59246/#review175004 --- Ship it! Ship It! - Ken Howe On May 12, 2017, 10:12 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59246/ > --- > > (Updated May 12, 2017, 10:12 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > * tests using CliCommandTestBase will set this flag to true which messes up > following test vms. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java > bc4db1660054ba837a4907d1173a254931fef230 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CliCommandTestBase.java > afdb706c285578f63fdaf459ac4ae59d83ab27e3 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/MemberCommandsDUnitTest.java > e41164495e2330ffdbf70bd52dc91bc0ae333830 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java > a5293eb0fa0a2599b837abc76903704a45428111 > > > Diff: https://reviews.apache.org/r/59246/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59210: GEODE-2912: Hot deploy for functions in deployed Jars
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59210/#review174826 --- Ship it! Ship It! - Ken Howe On May 12, 2017, 6:06 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59210/ > --- > > (Updated May 12, 2017, 6:06 p.m.) > > > Review request for geode. > > > Repository: geode > > > Description > --- > > GEODE-2912: Hot deploy for functions in deployed Jars > > - New versions of a function now deploy over top the old versions without an > intermediate undeploy > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/internal/DeployedJar.java acb7d22 > geode-core/src/main/java/org/apache/geode/internal/JarDeployer.java df3f10b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandRedeployDUnitTest.java > 6972477 > > > Diff: https://reviews.apache.org/r/59210/diff/2/ > > > Testing > --- > > Precheckin running > > > Thanks, > > Jared Stewart > >
Re: Review Request 59098: GEODE-2876: add logging to diagnose parser test failure in Jenkins
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59098/#review174330 --- Ship it! geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java Lines 52 (patched) <https://reviews.apache.org/r/59098/#comment247462> This is probably OK since the change is presumably temporary, and will be removed after diagnosing the parsing test failures. However, for style consistentcy I would have added org.apache.logging.log4j.Logger as an import rather than specifying the full class path in the declaration. - Ken Howe On May 9, 2017, 3:32 p.m., Jinmei Liao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59098/ > --- > > (Updated May 9, 2017, 3:32 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2876: add logging to diagnose test failure > > another try to add parser logs using log4j. I have another set of changes > that would remove the other loggers in GfshParser. For now, they co-exist. > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java > 20ae0226db45574028311493a07cdfcf9fbfb3f0 > > > Diff: https://reviews.apache.org/r/59098/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Jinmei Liao > >
Re: Review Request 59035: GEODE-2883: Fix GFSH gc heap size output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59035/#review174171 --- Ship it! Ship It! - Ken Howe On May 5, 2017, 11:19 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59035/ > --- > > (Updated May 5, 2017, 11:19 p.m.) > > > Review request for geode, Barry Oglesby, Jinmei Liao, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2883: Fix GFSH gc heap size output > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GarbageCollectionFunction.java > 354d353 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/BytesToString.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/BytesToStringTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59035/diff/3/ > > > Testing > --- > > Precheckin started (still running) > > > Thanks, > > Jared Stewart > >
Re: Review Request 59035: GEODE-2883: Fix GFSH gc heap size output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59035/#review174099 --- geode-core/src/test/java/org/apache/geode/management/internal/cli/util/BytesToStringTest.java Line 27 (original), 19 (patched) <https://reviews.apache.org/r/59035/#comment247206> Unused import? - Ken Howe On May 5, 2017, 11:08 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59035/ > --- > > (Updated May 5, 2017, 11:08 p.m.) > > > Review request for geode, Barry Oglesby, Jinmei Liao, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2883: Fix GFSH gc heap size output > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GarbageCollectionFunction.java > 354d353 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/BytesToString.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/BytesToStringTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59035/diff/2/ > > > Testing > --- > > Precheckin started (still running) > > > Thanks, > > Jared Stewart > >
Re: Review Request 59035: GEODE-2883: Fix GFSH gc heap size output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59035/#review174097 --- Fix it, then Ship it! geode-core/src/test/java/org/apache/geode/management/internal/cli/util/BytesToStringTest.java Lines 1 (patched) <https://reviews.apache.org/r/59035/#comment247203> package should come after the license comment geode-core/src/test/java/org/apache/geode/management/internal/cli/util/BytesToStringTest.java Lines 122-123 (patched) <https://reviews.apache.org/r/59035/#comment247204> Test doesn't belong in this class - Ken Howe On May 5, 2017, 11:08 p.m., Jared Stewart wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59035/ > --- > > (Updated May 5, 2017, 11:08 p.m.) > > > Review request for geode, Barry Oglesby, Jinmei Liao, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > --- > > GEODE-2883: Fix GFSH gc heap size output > > > Diffs > - > > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GarbageCollectionFunction.java > 354d353 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/util/BytesToString.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/BytesToStringTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59035/diff/2/ > > > Testing > --- > > Precheckin started (still running) > > > Thanks, > > Jared Stewart > >
Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.
> On May 5, 2017, 10:03 p.m., Ken Howe wrote: > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java > > Line 91 (original) > > <https://reviews.apache.org/r/58682/diff/5-7/?file=1702040#file1702040line94> > > > > We should be using log4j levels, so restore this line. All of the > > "fineEnabled" and "logger.fine" should be "logger.isTraceEnabled" and > > "logger.trace" > > > > Then remove all the local declarations of: > > > >LogWriter logger = cache.getLogger(); > > Kirk Lund wrote: > "fine" translates to isDebugEnabled and logger.debug > > "finer" and "finest" both translate to isTraceEnabled and logger.trace I stand corrected! I should have looked up the level translations instead of making a comment from memory. - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58682/#review174089 --- On May 5, 2017, 9:22 p.m., Patrick Rhomberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58682/ > --- > > (Updated May 5, 2017, 9:22 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, > and Swapnil Bawaskar. > > > Repository: geode > > > Description > --- > > Added unittests to capture failing behavior. > > Corrected buildTable shifting columns when adding values with additional keys. > > > Refactoring of DataCommandResult and DataCommandFunction > > -- > > The majority of changes to DataCommandFunction and DataCommandResult are > refactoring. Two ignored exceptions have been explicitly noted in the logger. > > - In DataCommandFunction:423, there's an empty if block. I imagine I should > remove that, since empty code is a waste. Looking for it, I couldn't find > the comment-hinted separate method, though. Anyone familiar with this corner > of the code know if that actuall happens? > > - Qualitative changes are in DataCommandResult.buildTable. Items in the > table are scanned to build an agggregate key set, and those items missing any > of these keys are padded with `MISSING_VALUE`. > > - I moved some of the more cumbersome blocks of code to subfunctions, but may > have done this more than necessary. Opinions? > > - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive > development / note the bug in GEODE-2662. Are there additional tests that > would fit naturally in this file? > > > Diffs > - > > geode-core/build.gradle f07444a > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java > 29d68bd > > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java > 423d781 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java > bb77466 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java > e72654e > > geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java > 1af6261 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java > 0e2a41e > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > ead1047 > > > Diff: https://reviews.apache.org/r/58682/diff/7/ > > > Testing > --- > > Previous precheckin returned fully green. Running with new refactoring but > expect no issues. (Common last words.) > > > Thanks, > > Patrick Rhomberg > >
Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58682/#review174089 --- geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java Line 91 (original) <https://reviews.apache.org/r/58682/#comment247196> We should be using log4j levels, so restore this line. All of the "fineEnabled" and "logger.fine" should be "logger.isTraceEnabled" and "logger.trace" Then remove all the local declarations of: LogWriter logger = cache.getLogger(); - Ken Howe On May 5, 2017, 9:22 p.m., Patrick Rhomberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58682/ > --- > > (Updated May 5, 2017, 9:22 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, > and Swapnil Bawaskar. > > > Repository: geode > > > Description > --- > > Added unittests to capture failing behavior. > > Corrected buildTable shifting columns when adding values with additional keys. > > > Refactoring of DataCommandResult and DataCommandFunction > > -- > > The majority of changes to DataCommandFunction and DataCommandResult are > refactoring. Two ignored exceptions have been explicitly noted in the logger. > > - In DataCommandFunction:423, there's an empty if block. I imagine I should > remove that, since empty code is a waste. Looking for it, I couldn't find > the comment-hinted separate method, though. Anyone familiar with this corner > of the code know if that actuall happens? > > - Qualitative changes are in DataCommandResult.buildTable. Items in the > table are scanned to build an agggregate key set, and those items missing any > of these keys are padded with `MISSING_VALUE`. > > - I moved some of the more cumbersome blocks of code to subfunctions, but may > have done this more than necessary. Opinions? > > - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive > development / note the bug in GEODE-2662. Are there additional tests that > would fit naturally in this file? > > > Diffs > - > > geode-core/build.gradle f07444a > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java > 29d68bd > > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java > 423d781 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java > bb77466 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java > e72654e > > geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java > 1af6261 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/GemfireDataCommandsDUnitTest.java > 0e2a41e > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > ead1047 > > > Diff: https://reviews.apache.org/r/58682/diff/7/ > > > Testing > --- > > Previous precheckin returned fully green. Running with new refactoring but > expect no issues. (Common last words.) > > > Thanks, > > Patrick Rhomberg > >
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review174050 --- Ship it! I have now finished reviewing the changes to to product code. No new comments or issues on this batch of diffs. - Ken Howe On May 3, 2017, 10:10 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 3, 2017, 10:10 p.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 0def5d283e359b1766fc90dfa05903526c672b0b > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/ParallelAsyncEventQueueImpl.java > e799880d715ed1fba65df305cf37e92f27db54ea > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/SerialAsyncEventQueueImpl.java > 9252dc745b848ea7a5aa8793a1fdb3a3c004c7ee > > geode-core/src/main/java/org/apache/geode/cache/client/ClientCacheFactory.java > e72cbff048b5a7ecb24594382d776feaa8e0f4bd > > geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientRegionFactoryImpl.java > ef676673046c32f68558f85516c674f7dcc6e982 > > geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionImpl.java > a494138316d8538f3aebeffb7075bb2db33b6532 > > geode-core/src/ma
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173972 --- geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java Line 6768 (original), 6589 (patched) <https://reviews.apache.org/r/5/#comment247070> typo: 2nd '{@code' should be '}' geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java Line 8918 (original), 8603 (patched) <https://reviews.apache.org/r/5/#comment247071> Was this intended to be 'ForceReattemptException ignore'? - Ken Howe On May 3, 2017, 10:10 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 3, 2017, 10:10 p.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 0def5d283e359b1766fc90dfa05903526c672b0b > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/ParallelAsyncEventQueueImpl.java > e799880d715ed1fba65df305cf37e92f27db54ea > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/SerialAsyncEventQueueImpl.java > 9252dc745b848ea7a5aa8793a1fdb3a3c004c7ee > > geode-core/src/main/java/org/apache/geode/cache/client/ClientCacheFactory.java > e72cbff048b5a7ecb24594382d776feaa8e0f4bd &g
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173942 --- geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java Line 672 (original), 676 (patched) <https://reviews.apache.org/r/5/#comment247014> Remove the "{@code" and the closing "}" to get the snippet formatted resaonably in hte javadoc geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java Line 2001 (original), 1960-1961 (patched) <https://reviews.apache.org/r/5/#comment247042> Swap these lines to keep the comments contiguous - Ken Howe On May 3, 2017, 10:10 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 3, 2017, 10:10 p.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 0def5d283e359b1766fc90dfa05903526c672b0b > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/ParallelAsyncEventQueueImpl.java > e799880d715ed1fba65df305cf37e92f27db54ea > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/SerialAsyncEventQueueImpl.java > 9252dc745b848ea7a5aa8793a1fdb3a3c004c7ee > > geode-core/src/main/java/org/apache/geode/
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173912 --- geode-core/src/main/java/org/apache/geode/internal/cache/DistTXPrecommitMessage.java Line 468 (original), 461 (patched) <https://reviews.apache.org/r/5/#comment246995> Seems inconsistent to rename this inner class, "...Precommit..." -> "...PreCommit..." when the class it's in remains as "...Precommit..." edit: This can probably be ignored if you've reverted the DistTX* changes - Ken Howe On May 3, 2017, 10:10 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 3, 2017, 10:10 p.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 0def5d283e359b1766fc90dfa05903526c672b0b > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/ParallelAsyncEventQueueImpl.java > e799880d715ed1fba65df305cf37e92f27db54ea > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/SerialAsyncEventQueueImpl.java > 9252dc745b848ea7a5aa8793a1fdb3a3c004c7ee > > geode-core/src/main/java/org/apache/geode/cache/client/ClientCacheFactory.java > e72cbff048b5a7ecb24594382d776feaa8e0f4bd > > ge
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173845 --- geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java Line 653 (original), 672 (patched) <https://reviews.apache.org/r/5/#comment246887> extra '//' after comment reformatting geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionEntry.java Line 1763 (original), 1746 (patched) <https://reviews.apache.org/r/5/#comment246892> Suggest rewording this comment "For a long time conflict checks were turned off ..." - Ken Howe On May 3, 2017, 10:10 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 3, 2017, 10:10 p.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 0def5d283e359b1766fc90dfa05903526c672b0b > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/ParallelAsyncEventQueueImpl.java > e799880d715ed1fba65df305cf37e92f27db54ea > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/SerialAsyncEventQueueImpl.java > 9252dc745b848ea7a5aa8793a1fdb3a3c004c7ee > > geode-core/src/main/java/org/apache/geode/cache/clie
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173839 --- geode-core/src/main/java/org/apache/geode/internal/admin/remote/RegionSubRegionsSizeResponse.java Line 98 (original), 104 (patched) <https://reviews.apache.org/r/5/#comment246877> FYI: Not something to fix at this time, but I've noticed that we have inconsistent spellings of "cancelled" throughout the project. Both canceled and cancelled are considered correct, and we use both of them. - Ken Howe On May 3, 2017, 10:10 p.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 3, 2017, 10:10 p.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueImpl.java > 0def5d283e359b1766fc90dfa05903526c672b0b > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/ParallelAsyncEventQueueImpl.java > e799880d715ed1fba65df305cf37e92f27db54ea > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/SerialAsyncEventQueueImpl.java > 9252dc745b848ea7a5aa8793a1fdb3a3c004c7ee > > geode-core/src/main/java/org/apache/geode/cache/client/ClientCacheFactory.java > e72cbff048b5a7ecb24594382d776feaa8e0f4bd > > geode-core/src/main/java/
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173821 --- geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java Lines 97-98 (original), 96-97 (patched) <https://reviews.apache.org/r/5/#comment246860> In the javadoc, missing a closing '}' and a ';' between the first two statments - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cache/TransactionDataRebalancedException.java > fded49f3c00772f38bac14bc57fa4614144930c1 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/internal/AsyncEventQueueFactoryImpl.java > 1a4052b27688c8e04addf6987ce39ea006f71cc5 > > geode-core/src/main/java/org/apache/geode/cache/asyncqueue/intern
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173809 --- geode-core/src/main/java/org/apache/geode/cache/query/internal/index/FunctionalIndexCreationHelper.java Line 105 (original), 111 (patched) <https://reviews.apache.org/r/5/#comment246848> typo? modiyy -> modify geode-core/src/main/java/org/apache/geode/cache/query/internal/index/FunctionalIndexCreationHelper.java Line 204 (original), 211 (patched) <https://reviews.apache.org/r/5/#comment246853> Pull the assingment to name outside the if, then use StringUtils.isEmpty. geode-core/src/main/java/org/apache/geode/cache/query/internal/index/FunctionalIndexCreationHelper.java Line 520 (original), 530 (patched) <https://reviews.apache.org/r/5/#comment246856> typo - correspnds to my note on line 111 - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/ge
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173786 --- geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java Lines 910-913 (original), 847-850 (patched) <https://reviews.apache.org/r/5/#comment246819> If I understand this change correctly, I think you can do away with the anonymous inner class. Originally the stack trace was explicitly printed, now with the empty inner class we won't get a stack trace at all. So just throw the new exception and let the default constructor fill in the stack trace and casue. If the intent was to not have the cause filled in then couldn't we just throw a new zero-arg CacheException? - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/RegionFactory.java > 3a2e9f68c96f9e4aae5a837e175014a3598f69ea > > geode-core/src/main/java/org/apache/geode/cac
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173740 --- geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java Line 148 (original), 151 (patched) <https://reviews.apache.org/r/5/#comment246781> Can use StringUtils.isEmpty(projectionAttributes) geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java Line 556 (original), 588 (patched) <https://reviews.apache.org/r/5/#comment246782> Delete commented out code, this diff already deletes the corresponding commented out declaration of lss. geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java Line 904 (original), 936 (patched) <https://reviews.apache.org/r/5/#comment246783> Suggest making this a javadoc geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java Line 1108 (original), 1142 (patched) <https://reviews.apache.org/r/5/#comment246785> Add tag to preserve paragraph formatting geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java Lines 1148-1149 (original), 1182-1183 (patched) <https://reviews.apache.org/r/5/#comment246786> Add {} around the if clause geode-core/src/main/java/org/apache/geode/cache/query/internal/index/AbstractIndex.java Line 1223 (original), 1256 (patched) <https://reviews.apache.org/r/5/#comment246788> typo: remove '//' that are left over from the original comment - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173644 --- geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java Line 425 (original), 419 (patched) <https://reviews.apache.org/r/5/#comment246648> but the exception is not ignored, so don't name it ignore geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java Line 580 (original), 559 (patched) <https://reviews.apache.org/r/5/#comment246651> Rename ignore. Another case where the exception is not ignored geode-core/src/main/java/org/apache/geode/cache/query/internal/ExecutionContext.java Line 182 (original), 180 (patched) <https://reviews.apache.org/r/5/#comment246659> I would leave the @link annotation intact geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java Line 358 (original), 359 (patched) <https://reviews.apache.org/r/5/#comment246682> Can you use a StringBHuilder here? geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 723 (original), 683 (patched) <https://reviews.apache.org/r/5/#comment246684> Incomplete TODO comment. The line that was deleted: // Object[] can be added rather than Struct geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 873 (patched) <https://reviews.apache.org/r/5/#comment246687> remove empty block comment geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 1148 (original), 1086 (patched) <https://reviews.apache.org/r/5/#comment246688> Typo: th -> the geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1430-1439 (original), 1341-1350 (patched) <https://reviews.apache.org/r/5/#comment246690> Formatting of this block of comments (and the one a dozen lines below) can be cleaned up geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1539-1541 (original), 1448-1450 (patched) <https://reviews.apache.org/r/5/#comment246691> comments canb be reformatted. geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1580-1586 (original), 1489-1495 (patched) <https://reviews.apache.org/r/5/#comment246692> more comment formatting geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1588-1593 (original), 1497-1502 (patched) <https://reviews.apache.org/r/5/#comment246693> more comment formatting - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173644 --- geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java Line 425 (original), 419 (patched) <https://reviews.apache.org/r/5/#comment246648> but the exception is not ignored, so don't name it ignore geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java Line 580 (original), 559 (patched) <https://reviews.apache.org/r/5/#comment246651> Rename ignore. Another case where the exception is not ignored geode-core/src/main/java/org/apache/geode/cache/query/internal/ExecutionContext.java Line 182 (original), 180 (patched) <https://reviews.apache.org/r/5/#comment246659> I would leave the @link annotation intact geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java Line 358 (original), 359 (patched) <https://reviews.apache.org/r/5/#comment246682> Can you use a StringBHuilder here? geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 723 (original), 683 (patched) <https://reviews.apache.org/r/5/#comment246684> Incomplete TODO comment. The line that was deleted: // Object[] can be added rather than Struct geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 873 (patched) <https://reviews.apache.org/r/5/#comment246687> remove empty block comment geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 1148 (original), 1086 (patched) <https://reviews.apache.org/r/5/#comment246688> Typo: th -> the geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1430-1439 (original), 1341-1350 (patched) <https://reviews.apache.org/r/5/#comment246690> Formatting of this block of comments (and the one a dozen lines below) can be cleaned up geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1539-1541 (original), 1448-1450 (patched) <https://reviews.apache.org/r/5/#comment246691> comments canb be reformatted. geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1580-1586 (original), 1489-1495 (patched) <https://reviews.apache.org/r/5/#comment246692> more comment formatting geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1588-1593 (original), 1497-1502 (patched) <https://reviews.apache.org/r/5/#comment246693> more comment formatting - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173644 --- geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java Line 425 (original), 419 (patched) <https://reviews.apache.org/r/5/#comment246648> but the exception is not ignored, so don't name it ignore geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java Line 580 (original), 559 (patched) <https://reviews.apache.org/r/5/#comment246651> Rename ignore. Another case where the exception is not ignored geode-core/src/main/java/org/apache/geode/cache/query/internal/ExecutionContext.java Line 182 (original), 180 (patched) <https://reviews.apache.org/r/5/#comment246659> I would leave the @link annotation intact geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryMonitor.java Line 358 (original), 359 (patched) <https://reviews.apache.org/r/5/#comment246682> Can you use a StringBHuilder here? geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 723 (original), 683 (patched) <https://reviews.apache.org/r/5/#comment246684> Incomplete TODO comment. The line that was deleted: // Object[] can be added rather than Struct geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 873 (patched) <https://reviews.apache.org/r/5/#comment246687> remove empty block comment geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 1148 (original), 1086 (patched) <https://reviews.apache.org/r/5/#comment246688> Typo: th -> the geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1430-1439 (original), 1341-1350 (patched) <https://reviews.apache.org/r/5/#comment246690> Formatting of this block of comments (and the one a dozen lines below) can be cleaned up geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1539-1541 (original), 1448-1450 (patched) <https://reviews.apache.org/r/5/#comment246691> comments canb be reformatted. geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1580-1586 (original), 1489-1495 (patched) <https://reviews.apache.org/r/5/#comment246692> more comment formatting geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Lines 1588-1593 (original), 1497-1502 (patched) <https://reviews.apache.org/r/5/#comment246693> more comment formatting - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173596 --- Posting my review of the next chunk of files geode-core/src/main/java/org/apache/geode/cache/client/ClientCacheFactory.java Line 227 (original), 232 (patched) <https://reviews.apache.org/r/5/#comment246582> Can be clarified using StringUtils.isEmpty(propvalue) geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java Line 222 (original), 242 (patched) <https://reviews.apache.org/r/5/#comment246583> Can use StringUtils.isEmpty(serverGroup). geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledIn.java Line 184 (original), 177 (patched) <https://reviews.apache.org/r/5/#comment246602> remove 'this.' geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledLike.java Line 39 (original), 37 (patched) <https://reviews.apache.org/r/5/#comment246605> Does logger need to be static? geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledLike.java Lines 39-62 (original), 37-62 (patched) <https://reviews.apache.org/r/5/#comment246606> As a matter of form, I would group the statics together (single spaced) and then the instance variables (also single spaced). geode-core/src/main/java/org/apache/geode/cache/query/internal/QueryUtils.java Line 1503 (original), 1409 (patched) <https://reviews.apache.org/r/5/#comment246589> For consistency, suggest renaming this method to getConditionedRelationshipIndexResultsExpandedToTopOrCGJLevel (similar to the renamed getConditionedIndexResults) - Ken Howe On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/Prepare
Re: Review Request 58888: GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache
> On May 1, 2017, 10:28 p.m., Ken Howe wrote: > > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > > Line 237 (original), 232 (patched) > > <https://reviews.apache.org/r/5/diff/1/?file=1704078#file1704078line237> > > > > {@code byteThreshold should be > > {@code byteThreshold} > > Kirk Lund wrote: > Yeah, looks like was ignored because I had "Match Case" checked > when I replaced with } > > I haven't found any others yet. What classes have these typos in them? The other places I was referring to are the extra lines in DynamicRegionFactory where you ended up with "code" inside of the \...\ tags - Ken --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5/#review173500 --- On May 2, 2017, 12:06 a.m., Kirk Lund wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5/ > --- > > (Updated May 2, 2017, 12:06 a.m.) > > > Review request for geode, Darrel Schneider, Jinmei Liao, Jared Stewart, Ken > Howe, and Patrick Rhomberg. > > > Bugs: GEODE-2632 > https://issues.apache.org/jira/browse/GEODE-2632 > > > Repository: geode > > > Description > --- > > GEODE-2632: change dependencies on GemFireCacheImpl to InternalCache > > * misc cleanup of code where possible > > I'm running full regression and precheckin. Sorry this is so many files -- > I'm not sure this is even practical to review :/ > > I'd like to avoid any further cleanup and just get this committed asap to > avoid conflicts. Let's hold off on throwing more changes on top of this if > you see more lines of code in a file that you'd like cleaned up. I obviously > want to fix problems in any code that I touched though! > > > Diffs > - > > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/common/CacheProperty.java > fe16fc3bd85e5861205dcdac6583d32abe57fcac > > extensions/geode-modules-session-internal/src/main/java/org/apache/geode/modules/session/internal/filter/GemfireSessionManager.java > 20cfecac2aaafd09c04eb6115a0da9c99201927b > > extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSessionManager.java > edc2b7d8bfc684257c27d90d4b8d0c4db378347d > > extensions/geode-modules/src/main/java/org/apache/geode/modules/util/CreateRegionFunction.java > 4e9e9fdf63eba622a157138e8e164956c257f093 > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCALocalTransaction.java > 112f2fa44478f00782bcb717d8cb75233c979dce > > geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java > 520f7e245ca3493d54ac661da29f58a3dfc71189 > geode-core/src/main/java/org/apache/geode/DataSerializer.java > 58518f4f5d43b3fe214a1218703360928efdee52 > geode-core/src/main/java/org/apache/geode/admin/GemFireMemberStatus.java > 5b4e59ef341007a4e42cd5a10b4e8a2eb4d18deb > > geode-core/src/main/java/org/apache/geode/admin/internal/CacheHealthEvaluator.java > f7ff9ede1e074425ce7aa7bed1a63c72c5174bf6 > > geode-core/src/main/java/org/apache/geode/admin/internal/FinishBackupRequest.java > 25abd7ee5d346d764d68339ae6c03b2cffc63bc8 > > geode-core/src/main/java/org/apache/geode/admin/internal/FlushToDiskRequest.java > ff6dd9d2088124203842a06d3a794e00a5636246 > > geode-core/src/main/java/org/apache/geode/admin/internal/MemberHealthEvaluator.java > 951b364718f43c5efc85c0f0f0e5d54e24d87ced > > geode-core/src/main/java/org/apache/geode/admin/internal/PrepareBackupRequest.java > 7025721180cb6b158cff4b21bfcea6549780ccd3 > geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java > 1a46f241a7b3a4bbbd47da70c227b3e3ea237fd0 > geode-core/src/main/java/org/apache/geode/cache/CacheClosedException.java > b24bc2fc5ad39737e908f9df2704b62728222629 > geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java > 0772dcf86b3d3ea9eebfe1d324c8fa043fa8ac79 > geode-core/src/main/java/org/apache/geode/cache/DynamicRegionFactory.java > 57a1a465c1bd0d91d8bb83ad2b97db1f2efbb025 > geode-core/src/main/java/org/apache/geode/cache/GemFireCache.java > e60bc59a88051ac11a863ea10be3e2bf07e4cbe7 > geode-core/src/main/java/org/apache/geode/cache/Region.java > 3eef5438e78a94acd669764e88daaafe6f5fa388 > geode-core/src/main/java/org/apache/geode/cache/Regi
Re: Review Request 58682: GEODE-2662: Missing keys cause columns to shift in gfsh table display.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58682/#review173528 --- Ship it! Ship It! - Ken Howe On April 27, 2017, 6:30 p.m., Patrick Rhomberg wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58682/ > --- > > (Updated April 27, 2017, 6:30 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, Kirk Lund, > and Swapnil Bawaskar. > > > Repository: geode > > > Description > --- > > Added unittests to capture failing behavior. > > Corrected buildTable shifting columns when adding values with additional keys. > > > Refactoring of DataCommandResult and DataCommandFunction > > -- > > The majority of changes to DataCommandFunction and DataCommandResult are > refactoring. Two ignored exceptions have been explicitly noted in the logger. > > - In DataCommandFunction:423, there's an empty if block. I imagine I should > remove that, since empty code is a waste. Looking for it, I couldn't find > the comment-hinted separate method, though. Anyone familiar with this corner > of the code know if that actuall happens? > > - Qualitative changes are in DataCommandResult.buildTable. Items in the > table are scanned to build an agggregate key set, and those items missing any > of these keys are padded with `MISSING_VALUE`. > > - I moved some of the more cumbersome blocks of code to subfunctions, but may > have done this more than necessary. Opinions? > > - Introduced DataCommandFunctionWithPDXJUnitTest to explicitly drive > development / note the bug in GEODE-2662. Are there additional tests that > would fit naturally in this file? > > > Diffs > - > > geode-core/build.gradle f07444a > > geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DataCommands.java > 6324b5c > > geode-core/src/main/java/org/apache/geode/management/internal/cli/domain/DataCommandResult.java > 423d781 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java > bb77466 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/result/TabularResultData.java > e72654e > > geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryDataInconsistencyDUnitTest.java > 1af6261 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/DataCommandFunctionWithPDXJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > ead1047 > > > Diff: https://reviews.apache.org/r/58682/diff/5/ > > > Testing > --- > > Previous precheckin returned fully green. Running with new refactoring but > expect no issues. (Common last words.) > > > Thanks, > > Patrick Rhomberg > >