> On June 27, 2017, 10:53 a.m., Galen O'Sullivan wrote: > > MembershipID seems like a good candidate for extraction into its own class. > > This would make, for example, `equals` cleaner. I guess there are > > performance reasons for it. > > > > When does the problem occur? Is it when a client reconnects with a newer > > version and the same ID? I wonder, would it be possible to make a check for > > that case specifically?
This is a blocker for the 1.2.0 release so no refactoring. The problem occurs when the ID is serialized using a versioned stream with a different version than that of the client. The client's version is not known in that code, which is one source of the problem. There are other places like this that need to be taken care of (VMCachedDeserializables in M&M code, for example) but that's not something to tackle here. > On June 27, 2017, 10:53 a.m., Galen O'Sullivan wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/EventID.java > > Lines 415 (patched) > > <https://reviews.apache.org/r/60446/diff/1/?file=1763222#file1763222line415> > > > > We've just checked whether the length is equal; does this assert > > actually check anything? The assert's already been removed (Kirk's comment) > On June 27, 2017, 10:53 a.m., Galen O'Sullivan wrote: > > geode-core/src/test/java/org/apache/geode/internal/cache/ha/ThreadIdentifierJUnitTest.java > > Lines 40 (patched) > > <https://reviews.apache.org/r/60446/diff/1/?file=1763224#file1763224line40> > > > > Is it possible for a Membership ID to be less than 17 bytes and this to > > be negative? No, that's not possible - Bruce ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60446/#review178935 ----------------------------------------------------------- On June 26, 2017, 3: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, 3: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 > >
