Skip to content

GEODE-6013: Use expected initial image requester's rvv information#2857

Merged
agingade merged 1 commit intoapache:developfrom
agingade:feature/GEODE-6013
Nov 29, 2018
Merged

GEODE-6013: Use expected initial image requester's rvv information#2857
agingade merged 1 commit intoapache:developfrom
agingade:feature/GEODE-6013

Conversation

@agingade
Copy link
Copy Markdown

Re-submitting after fixing NPE from previous checkin.

Made changes to use the expected initial image requester's rvv
information instead of the image provider's local rvv while
determining full or delta GII.

There was logical error where it was using provider's local
exception(rvv) instead of using requester's local exception(rvv).
This could result in performing Delta GII instead of Full GII.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • [Yes ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • [Yes ] Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • [Yes] Is your initial contribution a single, squashed commit?

  • [Yes] Does gradlew build run cleanly?

  • [Yes ] Have you written or updated unit tests to verify your changes?

  • [NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Re-submitting after fixing NPE from previous checkin.

Made changes to use the expected initial image requester's rvv
information instead of the image provider's local rvv while
determining full or delta GII.

There was logical error where it was using provider's local
exception(rvv) instead of using requester's local exception(rvv).
This could result in performing Delta GII instead of Full GII.
@bschuchardt
Copy link
Copy Markdown
Contributor

bschuchardt commented Nov 15, 2018

For other reviewers, here is a diff between Anil's last PR and this one:

  118c118
  < index 7b03d48bfc8..ce10dfdc1b3 100644
  ---
  > index 7b03d48bfc8..b53b5029b8d 100644
  121c121
  < @@ -977,34 +977,35 @@ public boolean isNewerThanOrCanFillExceptionsFor(RegionVersionVector<T> other) {
  ---
  > @@ -977,7 +977,8 @@ public boolean isNewerThanOrCanFillExceptionsFor(RegionVersionVector<T> other) {
  131,134c131
  <        RegionVersionHolder<T> holder = new RegionVersionHolder<T>(gcVersion.longValue());
  < -      return !holder.isNewerThanOrCanFillExceptionsFor(otherHolder);
  < +      return otherHolder.dominates(holder);
  <      }
  ---
  > @@ -987,24 +988,24 @@ private boolean isGCVersionDominatedByHolder(Long gcVersion, RegionVersionHolder
  184c181
  < index deb7d4eaa3d..a018aaff6fa 100644
  ---
  > index deb7d4eaa3d..ed671a6f09c 100644
  187c184
  < @@ -661,6 +661,145 @@ public void doesNotHangIfOtherThreadChangedVersion() throws Exception {
  ---
  > @@ -661,6 +661,167 @@ public void doesNotHangIfOtherThreadChangedVersion() throws Exception {
  235a233,254
  > +  public void isRvvGcDominatedByRequesterRvvReturnsFalseIfProviderRvvIsNotPresent()
  > +      throws Exception {
  > +    final String local = getIPLiteral();
  > +    InternalDistributedMember provider = new InternalDistributedMember(local, 101);
  > +    InternalDistributedMember requester = new InternalDistributedMember(local, 102);
  > +
  > +    RegionVersionVector providerRvv = new VMRegionVersionVector(provider, null,
  > +        1, null, 1, false, null);
  > +
  > +    ConcurrentHashMap<InternalDistributedMember, RegionVersionHolder<InternalDistributedMember>> memberToRegionVersionHolder =
  > +        new ConcurrentHashMap<>();
  > +    RegionVersionHolder regionVersionHolder = new RegionVersionHolder(provider);
  > +    regionVersionHolder.setVersion(0);
  > +    // memberToRegionVersionHolder.put(provider, regionVersionHolder);
  > +    RegionVersionVector requesterRvv =
  > +        new VMRegionVersionVector(requester, memberToRegionVersionHolder,
  > +            0, null, 0, false, null);
  > +
  > +    assertThat(providerRvv.isRVVGCDominatedBy(requesterRvv)).isFalse();
  > +  }
  > +
  > +  @Test

Copy link
Copy Markdown
Contributor

@bschuchardt bschuchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Anil, can you explain what the difference is between this PR and your last one? I see that you have a new test but the RVV changes seem exactly the same to me. You've changed one line to use dominates() instead of isNewerThanOrCanFillExceptionsFor() but the effect seems the same to me given the implementation of RVH.dominates().

  public boolean dominates(RegionVersionHolder<T> other) {
    return !other.isNewerThanOrCanFillExceptionsFor(this);
  }

Bruce, as part of the fix, i was trying to refactor the code to make it more readable. In the new checkin there is no logical changes except reverting back to its original code.

In my previous checkin, in "isGCVersionDominatedByHolder()" i had changed:
return !holder.isNewerThanOrCanFillExceptionsFor(otherHolder);
TO
return otherHolder.dominates(holder);

Idea was to change from negate condition; which caused NPE as "otherHolder" could be null. To address NPE, I reverted back to the original code.

@agingade agingade merged commit b40303d into apache:develop Nov 29, 2018
@agingade agingade deleted the feature/GEODE-6013 branch November 29, 2018 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants