----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60446/#review179014 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java Lines 396 (patched) <https://reviews.apache.org/r/60446/#comment253413> Is there any way at all we can make the code explain itself enough so that we don't need the comment repeated three times? In all three cases we are ensuring that a difference is smaller than the Member data length. Maybe a method could be introduced that has a self explanatory name? Maybe something like `compareSignificantByteSize(byte[] memberID, int otherSize)`? Maybe we even could have a class MemberID that is responsible for this functionality. geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java Lines 437 (patched) <https://reviews.apache.org/r/60446/#comment253402> Could the "19" be demagicalized by introducing a well named constant instead of a comment? - Alexander Murmann On June 26, 2017, 10:24 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, 10:24 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/2/ > > > Testing > ------- > > > File Attachments > ---------------- > > diffall.txt > > https://reviews.apache.org/media/uploaded/files/2017/06/26/98d794d9-6e21-4a1b-8ee5-2394ac2baa6f__diffall.txt > diffall.txt > > https://reviews.apache.org/media/uploaded/files/2017/06/26/cc4dfcc9-dc0b-48da-b97b-337563b127fe__diffall.txt > diffall.txt > > https://reviews.apache.org/media/uploaded/files/2017/06/26/de1405e6-38f0-4972-adf7-e43e568ff5ad__diffall.txt > diffall.txt > > https://reviews.apache.org/media/uploaded/files/2017/06/26/8f143bb9-0225-4e3b-ace1-09614cf5efe8__diffall.txt > > > Thanks, > > Bruce Schuchardt > >
