I think Jinmei might be right. For the case of version == this.version + 1, new code will call addOldVersion(). It will not do anything, but it's different with old hehavior. In old logic, addOldVersion() will not be called.
On Tue, Jan 19, 2016 at 8:57 AM, Jinmei Liao <jil...@pivotal.io> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42465/ > > On January 19th, 2016, 4:40 p.m. UTC, *Jinmei Liao* wrote: > > > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/RegionVersionHolder.java > <https://reviews.apache.org/r/42465/diff/2/?file=1200643#file1200643line331> > (Diff > revision 2) > > private void addBitSetExceptions(int numBits, long newVersion) { > > 320 > > if (this.version != version) { > > 331 > > private void recordVersionWithoutBitSet(long version) { > > Looks like in the recordVersionWithoutBitSet, in case when version == > this.version and version= this.version + 1, we are still calling > this.addOlderVersion(version). Would this have any unexpected impact on the > end result? > > On January 19th, 2016, 4:46 p.m. UTC, *Bruce Schuchardt* wrote: > > I think that's intentional, for the "special exception" - though I'm still a > bit unclear about what this exception is supposed to be. That's what the old > code was doing, in any case, and the refactoring is supposed to preserve the > old behavior. > > Oh, by looking more closely to the old behavior, in the case of > "version==this.version" it is calling addOlderVersion, but in the case > (version == this.version+1), it does not look like it's doing that......., or > maybe I am wrong. Just so that you can take a look. Thanks! > > > - Jinmei > > On January 18th, 2016, 6:29 p.m. UTC, Bruce Schuchardt wrote: > Review request for geode, Dan Smith and xiaojian zhou. > By Bruce Schuchardt. > > *Updated Jan. 18, 2016, 6:29 p.m.* > *Repository: * geode > Description > > This is from a group refactoring session during a class taught by Michael > Feathers. The old recordVersion code was difficult to understand and was > poorly tested. New JUnit tests are now included that give 100% code coverage > for this algorithm. > > Testing > > precheckin including new tests > > Diffs > > - > gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/RegionVersionHolder.java > (4872df2a06c0bf6c949204cf98074d9259d6413f) > - > gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/RegionVersionHolder2JUnitTest.java > (PRE-CREATION) > > View Diff <https://reviews.apache.org/r/42465/diff/> >
