You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by pdxrunner <gi...@git.apache.org> on 2016/05/17 18:08:18 UTC
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
GitHub user pdxrunner opened a pull request:
https://github.com/apache/incubator-geode/pull/145
GEODE-1296: change conditional in getRawBytes to assert
Made suggested change to getRawBytes. Updated corresponding unit test.
See review request https://reviews.apache.org/r/47479/
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pdxrunner/incubator-geode feature/GEODE-1296
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-geode/pull/145.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #145
----
commit 538e0483d3e6fa7e04f5834be27d5540fbda792f
Author: Ken Howe <kh...@pivotal.io>
Date: 2016-05-12T22:04:16Z
GEODE-1296: change conditional in getRawBytes to assert
Updated correspondiong unit test.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by dschneider-pivotal <gi...@git.apache.org>.
Github user dschneider-pivotal commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/145#discussion_r63930600
--- Diff: geode-core/src/test/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObjectJUnitTest.java ---
@@ -42,6 +42,10 @@
@Category(UnitTest.class)
public class OffHeapStoredObjectJUnitTest extends AbstractStoredObjectTestBase {
+ static {
--- End diff --
Remove this static block. I talked with Kirk and we think that java assertions are enabled consistently from both gradle and the IDE
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by dschneider-pivotal <gi...@git.apache.org>.
Github user dschneider-pivotal commented on the pull request:
https://github.com/apache/incubator-geode/pull/145#issuecomment-221650434
The travis failure is a know intermittent issue in a new unit test.
This change looks good. I will pull it in.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by dschneider-pivotal <gi...@git.apache.org>.
Github user dschneider-pivotal commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/145#discussion_r63930434
--- Diff: geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObject.java ---
@@ -417,11 +417,13 @@ public void setSerializedValue(byte[] value) {
MemoryAllocatorImpl.getAllocator().getStats().incReads();
return result;
}
+ /**
+ * This method should only be called on uncompressed objects
+ * @return byte array of the StoredObject value.
+ */
protected byte[] getRawBytes() {
+ assert !isCompressed();
byte[] result = getCompressedBytes();
--- End diff --
Combine these two lines into "return getCompressedBytes();"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on the pull request:
https://github.com/apache/incubator-geode/pull/145#issuecomment-221722725
This was checked in by dschneider. Closing the PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by dschneider-pivotal <gi...@git.apache.org>.
Github user dschneider-pivotal commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/145#discussion_r63931246
--- Diff: geode-core/src/test/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObjectJUnitTest.java ---
@@ -816,6 +820,7 @@ public void getRawBytesShouldThrowExceptionIfValueIsCompressed() {
chunk.getRawBytes();
chunk.release();
+ fail("Expected getRawBytes() for a compressed value to throw java.lang.AssertionError");
--- End diff --
Why is this "fail" call added. Doesn't the "expected" on the Test annotation take care of this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by dschneider-pivotal <gi...@git.apache.org>.
Github user dschneider-pivotal commented on the pull request:
https://github.com/apache/incubator-geode/pull/145#issuecomment-220090005
I think this should be changed to use the java "assert" keyword. The assertion should only be checked when assertions on the jvm are enabled.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner closed the pull request at:
https://github.com/apache/incubator-geode/pull/145
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request: GEODE-1296: change conditional in ge...
Posted by pdxrunner <gi...@git.apache.org>.
Github user pdxrunner commented on the pull request:
https://github.com/apache/incubator-geode/pull/145#issuecomment-221624715
Precheckin passed with my most recent update. I think this one is ready to go now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---