----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60446/#review178926 -----------------------------------------------------------
Fix it, then Ship it! Fix it (or drop it) and then ship it! geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java Line 3491 (original), 3491 (patched) <https://reviews.apache.org/r/60446/#comment253251> You might want to guard the for-loop with: '''java if (logger.isTraceEnabled()) { ``` The JIT will hopefully optimize out the for-loop entirely when run at info-level. If dumpMap() is only called from a test then it probably doesn't matter either way. geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java Lines 415 (patched) <https://reviews.apache.org/r/60446/#comment253252> I would expect to see this assert at the beginning of the method. geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java Lines 438 (patched) <https://reviews.apache.org/r/60446/#comment253254> Delete commented-out code? geode-core/src/main/java/org/apache/geode/internal/cache/ha/ThreadIdentifier.java Lines 161 (patched) <https://reviews.apache.org/r/60446/#comment253255> Delete commented-out code? geode-core/src/main/java/org/apache/geode/internal/cache/ha/ThreadIdentifier.java Lines 203 (patched) <https://reviews.apache.org/r/60446/#comment253253> Extra ";" here and at the end of the above line. Probably delete one. geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscBCDUnitTest.java Line 100 (original), 101 (patched) <https://reviews.apache.org/r/60446/#comment253256> Delete commented-out code? - Kirk Lund On June 26, 2017, 8:45 p.m., Bruce Schuchardt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60446/ > ----------------------------------------------------------- > > (Updated June 26, 2017, 8:45 p.m.) > > > Review request for geode and Barry Oglesby. > > > Bugs: GEODE-3072 > https://issues.apache.org/jira/browse/GEODE-3072 > > > Repository: geode > > > Description > ------- > > EventID and ThreadIdentifier hold the serialized form of portions of an > InternalDistributedMember identifier. This serialized form changed in v1.0.0 > and was causing .equals and .hashCode for these objects to return false when > they should have returned true owing to additional data being in the > serialized form of the identifier. > > This change set modifies the equals and hashCode methods of the classes to > ensure that they return the correct results in the presence of this > additional ID data. > > I created a dunit test to reproduce the problem but I think the behavior of > HARegionQueues isn't reliable enough to check in that test. I'm afraid that > it would end up being a "flaky" test that fails periodically. Instead, I'm > relying on a new junit test and the test that Barry already checked in. > > Note: I've also included two other things in this change set. > > First, LocalRegion.dumpBackingMap() is a test-hook that should log its > results at "info" level. I used it in debugging this problem. > > Second, I've added a new backward-compatibility version so now we have 100 > and 110. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java > a1b4a9d5684d0148bd9e72c00c674afa299b2b9d > geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java > 4d2ddc11bb1bd36446dc30b2033a6bc1ed455c98 > > geode-core/src/main/java/org/apache/geode/internal/cache/ha/ThreadIdentifier.java > ec165a5af210966241fdc1cee81702231557cc8c > > geode-core/src/test/java/org/apache/geode/internal/cache/ha/ThreadIdentifierJUnitTest.java > 29b22be6cc866217f165b755f11e68e1343843fe > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscBCDUnitTest.java > 5fb8fa201b4c059d5d458a6af0915273f600f69f > geode-old-versions/build.gradle d85eb0b7dea6d3fa6b19a0030e64433cb4cb7cb9 > > > Diff: https://reviews.apache.org/r/60446/diff/1/ > > > Testing > ------- > > > Thanks, > > Bruce Schuchardt > >
