Mailing-List: contact dev-help@geode.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geode.apache.org
MIME-Version: 1.0
In-Reply-To: <CAJCfvb5_AsRnn_G97EnyRMrvZi0pTjZo9hbSfXYuf_EEm_b8zQ@mail.gmail.com>
References: <CAPN5c57BV6iAXr7CFyvwDRGCcM72QzdUJft2vgzGvDa94WLBnA@mail.gmail.com>
 <CAJrNg03Nk-AxHWKjftT9-CgSW7J1nBEk01m=ugVO0p+G=bhyCg@mail.gmail.com>
 <CAJCfvb6hM2FZUFHwDAXYWFZJv=eVY1WUEVs4_+SkpnAaMvyEJQ@mail.gmail.com> <CAJCfvb5_AsRnn_G97EnyRMrvZi0pTjZo9hbSfXYuf_EEm_b8zQ@mail.gmail.com>
From: Suranjan Kumar <suranjan.kumar@gmail.com>
Date: Wed, 24 May 2017 00:24:26 +0530
Message-ID: <CAPN5c55DTFPLzkR1LQ4OMg4CBg3GwcMtdZXoWjXjC5pEkXwO2w@mail.gmail.com>
Subject: Re: Issues due to Special exceptions in RVV
To: dev@geode.apache.org
Content-Type: multipart/alternative; boundary="001a1142263a554694055035848d"
archived-at: Tue, 23 May 2017 18:54:39 -0000

--001a1142263a554694055035848d
Content-Type: text/plain; charset="UTF-8"

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 <dschneider@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 <dschneider@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 <klund@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.kumar@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
> >> >
> >>
> >
> >
>

--001a1142263a554694055035848d--