Yes, please file a jira and share your fix.
We are wondering if in delta GII it would just cause some extra entries to
be sent.
In that case it will just impact performance but not data consistency.
What kind of issues are you seeing?
On Tue, May 23, 2017 at 11:54 AM, Suranjan Kumar <suranjan.ku...@gmail.com>
wrote:
> Hi Darrel,
> Actually, I reproduced the issue in unit test that was seen in one of the
> runs and initializeVersionHolder doesn't fix the issue.
> The initializeFrom is used to initialize a member's RVV from a GII
> provider.
>
> It turns out if a member has already recorded higher version then while
> initializing a version holder from a member with lower version, we
> introduce an special exception.
> But in future when new versions are recorded by the same member, the
> special exception is not handled properly and that leads to version holder
> data structure corruption leading to holder#contains(version) error. In
> fact, I see that in this case an exception can remain permanently in the
> version holder even though the version has been recorded.
>
> I have a fix, which I will send for review soon. However, I just wanted
> to understand the issue and why it is not caught in delta GII.
>
> Moreover, if you look at the method initializeFrom, then already recorded
> versions are ignored? Isn't that an issue? If not then why?
> I will file a JIRA if you suggest.
>
> On Tue, May 23, 2017 at 10:49 PM, Darrel Schneider <dschnei...@pivotal.io>
> wrote:
>
> > I see that this test directly calls initializeFrom but the product never
> > does this.
> > The product always calls it through this method:
> >
> > org.apache.geode.internal.cache.versions.RegionVersionVector.
> > initializeVersionHolder(T,
> > RegionVersionHolder<T>)
> >
> > I can see that initializeVersionHolder checks a memberToVersion map and
> on
> > a miss does not call initializeFrom but instead add the new member's RVV
> to
> > the memberToVersion map. Have you considered if calling
> > initializeVersionHolder would fix the issues you are seeing?
> >
> >
> > On Tue, May 23, 2017 at 10:04 AM, Darrel Schneider <
> dschnei...@pivotal.io>
> > wrote:
> >
> > > I made these modifications and the following compiles, runs, and fails
> on
> > > geode.
> > > I added the following to RegionVersionVectorJUnitTest:
> > > @Test
> > > public void testInitialized() {
> > >
> > > RegionVersionHolder<String> vh1 = new RegionVersionHolder<>("vh1");
> > > vh1.recordVersion(56);
> > > vh1.recordVersion(57);
> > > vh1.recordVersion(58);
> > > vh1.recordVersion(59);
> > > vh1.recordVersion(60);
> > > assertTrue(vh1.contains(57));
> > > vh1 = vh1.clone();
> > > assertTrue(vh1.contains(57));
> > > System.out.println("This node init, vh1="+vh1);
> > >
> > > RegionVersionHolder<String> vh2 = new RegionVersionHolder<>("vh2");
> > > for(int i=1;i<57;i++) {
> > > vh2.recordVersion(i);
> > > }
> > > vh2 = vh2.clone();
> > > System.out.println("GII node init, vh2="+vh2);
> > >
> > > vh1.initializeFrom(vh2);
> > > // assertTrue(vh1.contains(57)); // fails
> > > vh1 = vh1.clone();
> > > System.out.println("After initialize, vh1="+vh1);
> > >
> > > vh1.recordVersion(58);
> > > vh1.recordVersion(59);
> > > vh1.recordVersion(60);
> > >
> > > System.out.println("After initialize and record version,
> vh1="+vh1);
> > >
> > > vh1 = vh1.clone();
> > > vh1.recordVersion(57);
> > > System.out.println("After initialize and record version after
> clone,
> > > vh1="+vh1);
> > >
> > > assertTrue(vh1.contains(57)); //FAILS
> > > }
> > >
> > >
> > > On Tue, May 23, 2017 at 9:37 AM, Kirk Lund <kl...@apache.org> wrote:
> > >
> > >> Are you sure you're using Geode? The signature of
> > >> RegionVersionHolder#recordVersion
> > >> in Geode is:
> > >>
> > >> RegionVersionHolder#recordVersion(long)
> > >>
> > >> I recommend checking out develop branch of Geode, write a test to
> > confirm
> > >> your bug and then submit that test with a Jira ticket.
> > >>
> > >> On Tue, May 23, 2017 at 7:10 AM, Suranjan Kumar <
> > suranjan.ku...@gmail.com
> > >> >
> > >> wrote:
> > >>
> > >> > Hi Bruce/Dan,
> > >> >
> > >> > #1. I see some issues due to introduction of special exceptions in
> > the
> > >> > RegionVersionHolder. In some cases, it is leading to
> > RegionVersionHolder
> > >> > data structure corruption causing wrong contains(version) results.
> > >> >
> > >> > This happens because we set the version to max of currentVersion or
> > >> newly
> > >> > recorded one. But do not try to fix the special exception present if
> > >> any.
> > >> >
> > >> > It is easily reproducible even in junit tests.
> > >> > The corruption of holder data stricture causes even future record
> > >> operation
> > >> > to fail.
> > >> > For example the following test fails:
> > >> >
> > >> > public void testInitialized() {
> > >> >
> > >> > RegionVersionHolder vh1 = new RegionVersionHolder(member);
> > >> > vh1.recordVersion(56, null);
> > >> > vh1.recordVersion(57, null);
> > >> > vh1.recordVersion(58, null);
> > >> > vh1.recordVersion(59, null);
> > >> > vh1.recordVersion(60, null);
> > >> > vh1 = vh1.clone();
> > >> > System.out.println("This node init, vh1="+vh1);
> > >> >
> > >> > RegionVersionHolder vh2 = new RegionVersionHolder(member);
> > >> > for(int i=1;i<57;i++) {
> > >> > vh2.recordVersion(i,null);
> > >> > }
> > >> > vh2 = vh2.clone();
> > >> > System.out.println("GII node init, vh2="+vh2);
> > >> >
> > >> > vh1.initializeFrom(vh2);
> > >> > vh1 = vh1.clone();
> > >> > System.out.println("After initialize, vh1="+vh1);
> > >> >
> > >> >
> > >> > vh1.recordVersion(58,null);
> > >> > vh1.recordVersion(59,null);
> > >> > vh1.recordVersion(60,null);
> > >> >
> > >> > System.out.println("After initialize and record version,
> vh1="+vh1);
> > >> >
> > >> > vh1 = vh1.clone();
> > >> > vh1.recordVersion(57,null);
> > >> > System.out.println("After initialize and record version after
> clone,
> > >> > vh1="+vh1);
> > >> >
> > >> > assertTrue(vh1.contains(57)); //FAILS
> > >> > }
> > >> >
> > >> >
> > >> > #2. I have observed that
> > >> > RegionVersionHolder#initializeFrom(RegionVersionHolder<T> source)
> > >> > doesn't not record already recorded version in itself contrary to
> what
> > >> > the comment above the method claims. In fact the junit tests also
> > >> > verifies the same so I couldn't understand the rationale behind it.
> It
> > >> > just adds an special exception if any higher version was already
> > >> > recorded by self.
> > >> > Shouldn't the resulting holder after initializing from a source
> > >> > contain records for both the holders? If not then why do we even
> need
> > >> > special exception.
> > >> >
> > >> > If possible could you please let me know your thoughts on this.
> > >> >
> > >> > Regards,
> > >> > Suranjan Kumar
> > >> >
> > >>
> > >
> > >
> >
>
