You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "demery-pivotal (GitHub)" <gi...@apache.org> on 2019/01/11 19:49:02 UTC

[GitHub] [geode] demery-pivotal opened pull request #3068: Extract StatisticsRegistry from InternalDistributedSystem

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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [x] Is your initial contribution a single, squashed commit?

- [x] Does `gradlew build` run cleanly?

- [x] Have you written or updated unit tests to verify your changes?

- [x] 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.

Please review @kirklund @galen-pivotal @mhansonp 

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

[GitHub] [geode] kirklund commented on issue #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
The UpgradeTests have a bunch of failures that appear to be in just the Lucene tests and caused by the old version not having the method GemFireCacheImpl.getStatisticsRegistry(). I think this is test code blowing up and not product code. I think the test is trying to get some statistics to look at and is using test code that only works against the new version of product code. So we may need to change test code to fix this.

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

[GitHub] [geode] kirklund commented on issue #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
This is some pretty bad code in LuceneSearchWithRollingUpgradeDUnit.java. Apparently the line that fails is the one with `getMethod("getStatisticsRegistry")` and it looks like we incorrectly changed it from `"getDistributedSystem"`:
```
  private static void assertVersion(Object cache, short ordinal) throws Exception {
    Class idmClass = Thread.currentThread().getContextClassLoader()
        .loadClass("org.apache.geode.distributed.internal.membership.InternalDistributedMember");
    Method getDSMethod = cache.getClass().getMethod("getStatisticsRegistry");
    getDSMethod.setAccessible(true);
    Object ds = getDSMethod.invoke(cache);

    Method getDistributedMemberMethod = ds.getClass().getMethod("getDistributedMember");
    getDistributedMemberMethod.setAccessible(true);
    Object member = getDistributedMemberMethod.invoke(ds);
    Method getVersionObjectMethod = member.getClass().getMethod("getVersionObject");
    getVersionObjectMethod.setAccessible(true);
    Object thisVersion = getVersionObjectMethod.invoke(member);
    Method getOrdinalMethod = thisVersion.getClass().getMethod("ordinal");
    getOrdinalMethod.setAccessible(true);
    short thisOrdinal = (Short) getOrdinalMethod.invoke(thisVersion);
    if (ordinal != thisOrdinal) {
      throw new Error(
          "Version ordinal:" + thisOrdinal + " was not the expected ordinal of:" + ordinal);
    }
  }
```

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

[GitHub] [geode] kirklund closed pull request #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
[ pull request closed by kirklund ]

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

[GitHub] [geode] kirklund commented on issue #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
This is some pretty bad code in LuceneSearchWithRollingUpgradeDUnit.java. Apparently the line that fails is the one with `getMethod("getStatisticsRegistry")` and it looks like we incorrectly changed it from `"getDistributedSystem"`:
```
  private static void assertVersion(Object cache, short ordinal) throws Exception {
    Class idmClass = Thread.currentThread().getContextClassLoader()
        .loadClass("org.apache.geode.distributed.internal.membership.InternalDistributedMember");
    **_Method getDSMethod = cache.getClass().getMethod("getStatisticsRegistry");_**
    getDSMethod.setAccessible(true);
    Object ds = getDSMethod.invoke(cache);

    Method getDistributedMemberMethod = ds.getClass().getMethod("getDistributedMember");
    getDistributedMemberMethod.setAccessible(true);
    Object member = getDistributedMemberMethod.invoke(ds);
    Method getVersionObjectMethod = member.getClass().getMethod("getVersionObject");
    getVersionObjectMethod.setAccessible(true);
    Object thisVersion = getVersionObjectMethod.invoke(member);
    Method getOrdinalMethod = thisVersion.getClass().getMethod("ordinal");
    getOrdinalMethod.setAccessible(true);
    short thisOrdinal = (Short) getOrdinalMethod.invoke(thisVersion);
    if (ordinal != thisOrdinal) {
      throw new Error(
          "Version ordinal:" + thisOrdinal + " was not the expected ordinal of:" + ordinal);
    }
  }
```

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

[GitHub] [geode] kirklund commented on issue #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
This is some pretty bad code in LuceneSearchWithRollingUpgradeDUnit.java. Apparently the line that fails is the one with `getMethod("getStatisticsRegistry")` and it looks like we incorrectly changed it from `"getDistributedSystem"`:
```
  private static void assertVersion(Object cache, short ordinal) throws Exception {
    Class idmClass = Thread.currentThread().getContextClassLoader()
        .loadClass("org.apache.geode.distributed.internal.membership.InternalDistributedMember");
    Method getDSMethod = cache.getClass().getMethod("getStatisticsRegistry");
    getDSMethod.setAccessible(true);
    Object ds = getDSMethod.invoke(cache);

    Method getDistributedMemberMethod = ds.getClass().getMethod("getDistributedMember");
    getDistributedMemberMethod.setAccessible(true);
    Object member = getDistributedMemberMethod.invoke(ds);
    Method getVersionObjectMethod = member.getClass().getMethod("getVersionObject");
    getVersionObjectMethod.setAccessible(true);
    Object thisVersion = getVersionObjectMethod.invoke(member);
    Method getOrdinalMethod = thisVersion.getClass().getMethod("ordinal");
    getOrdinalMethod.setAccessible(true);
    short thisOrdinal = (Short) getOrdinalMethod.invoke(thisVersion);
    if (ordinal != thisOrdinal) {
      throw new Error(
          "Version ordinal:" + thisOrdinal + " was not the expected ordinal of:" + ordinal);
    }
  }
```

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

[GitHub] [geode] kirklund commented on pull request #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
Restore this line back to "getDistributedSystem". I'm not sure why IntelliJ changed this during the refactoring.

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

[GitHub] [geode] kirklund commented on issue #3068: GEODE-6269: Extract StatisticsRegistry from InternalDistributedSystem

Posted by "kirklund (GitHub)" <gi...@apache.org>.
This is some pretty bad code in LuceneSearchWithRollingUpgradeDUnit.java. Apparently the line that fails is this -- I think we must have accidentally changed this to "getStatisticsRegistry" on the PR branch and simply need to change it back:
```
Method getDSMethod = cache.getClass().getMethod("getDistributedSystem");
```
```
  private static void assertVersion(Object cache, short ordinal) throws Exception {
    Class idmClass = Thread.currentThread().getContextClassLoader()
        .loadClass("org.apache.geode.distributed.internal.membership.InternalDistributedMember");
    Method getDSMethod = cache.getClass().getMethod("getDistributedSystem");
    getDSMethod.setAccessible(true);
    Object ds = getDSMethod.invoke(cache);

    Method getDistributedMemberMethod = ds.getClass().getMethod("getDistributedMember");
    getDistributedMemberMethod.setAccessible(true);
    Object member = getDistributedMemberMethod.invoke(ds);
    Method getVersionObjectMethod = member.getClass().getMethod("getVersionObject");
    getVersionObjectMethod.setAccessible(true);
    Object thisVersion = getVersionObjectMethod.invoke(member);
    Method getOrdinalMethod = thisVersion.getClass().getMethod("ordinal");
    getOrdinalMethod.setAccessible(true);
    short thisOrdinal = (Short) getOrdinalMethod.invoke(thisVersion);
    if (ordinal != thisOrdinal) {
      throw new Error(
          "Version ordinal:" + thisOrdinal + " was not the expected ordinal of:" + ordinal);
    }
  }
```

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