You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "agingade (GitHub)" <gi...@apache.org> on 2018/11/09 00:27:07 UTC

[GitHub] [geode] agingade opened pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

GEODE-6013: Made changes to use the expected initial image requester's rvv informationrequester'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.

### 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](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

How about changing "GotRegionIIRequest" into "GotRIM" or "GotRequestImageMessage"?

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "kirklund (GitHub)" <gi...@apache.org>.

Compare the join() method and the await(). Then change this to:
```
vm0createRegion.await();
```

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "agingade (GitHub)" <gi...@apache.org>.

Changes made in new commit.

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade closed pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "agingade (GitHub)" <gi...@apache.org>.

[ pull request closed by agingade ]

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "agingade (GitHub)" <gi...@apache.org>.

Changes made in new commit to verify the number of delta and full giis done.

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "agingade (GitHub)" <gi...@apache.org>.

ErrorCollector is not working with bounced vm. Myself and Kirk looked at it and decided to keep the changes as is.

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "kirklund (GitHub)" <gi...@apache.org>.

beforeProcessMessage is invoked by a non-main thread, which means if this await() times out which throws an Exception, the test will only fail if grep-for-suspect-strings catches it. Grep should find the logged Exception and fail it, but a better approach is this:

Add SharedErrorCollector rule:
```
import org.apache.geode.test.dunit.rules.SharedErrorCollector;

@Rule
public SharedErrorCollector errorCollector = new SharedErrorCollector();
```
And then use errorCollector in any callback you've implemented such as beforeProcessMessage:
```
public void beforeProcessMessage(ClusterDistributionManager dm, DistributionMessage message) {
  try {
    ...
  } catch (RuntimeException | Error e) {
    sharedErrorCollector.addError(e);
  }
}
```
ErrorCollector is a standard JUnit Rule, and SharedErrorCollector is special DUnit version that you can use from within any DUnit VM including the controller VM.

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.

Nitpick:  Revert this method name change.  Version vectors don't know anything about "requesters".  Dominance is a mathematical vector operation.

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "agingade (GitHub)" <gi...@apache.org>.

Its to identify the blackboard signal; will leave it as it is.

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] bschuchardt commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "bschuchardt (GitHub)" <gi...@apache.org>.

good catch

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] gesterzhou commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "gesterzhou (GitHub)" <gi...@apache.org>.

The test did not verify it's a fullGII or deltaGII. 

We can get that information from stats:

        CachePerfStats stats = lr.getRegionPerfStats();

        // we saved GII completed count in RegionPerfStats only
        int size = stats.getGetInitialImageKeysReceived();
        cache.getLogger().info("Delta contains: " + size + " keys");
        assertEquals(expectedKeyNum, size);

        int num = stats.getDeltaGetInitialImagesCompleted();
        cache.getLogger().info("Delta GII completed: " + num + " times");
        assertEquals(expectedDeltaGIINum, num);


[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] agingade commented on pull request #2819: GEODE-6013: Made changes to use the expected initial image requester's rvv information

Posted by "agingade (GitHub)" <gi...@apache.org>.

The naming was confusing and harder to understand what it was doing; thought identifying the RVV from requesters to providers will help.
Reverted the method name changes in commit3

[ Full content available at: https://github.com/apache/geode/pull/2819 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org