You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by upthewaterspout <gi...@git.apache.org> on 2016/12/15 18:57:08 UTC
[GitHub] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
GitHub user upthewaterspout opened a pull request:
https://github.com/apache/geode/pull/318
Handle exceptions and don't deserialize PDX objects when creating indexes
These are two related changes to our index creation code. We should not deserialize PDX objects during index creation. We should also fail the index creation and clean up the index if there is a failure.
I'm a committer, I can merge these changes. Creating the PR for review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/upthewaterspout/incubator-geode feature/GEODE-1272
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/318.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 #318
----
commit 6782e0f100dacb5034e41f3854be27060f8af2e8
Author: Dan Smith <up...@apache.org>
Date: 2016-12-15T00:59:51Z
GEODE-2216: Throwing an exception if index creation fails.
Making sure index creation always throws an exception and cleans up the
index if the index creation fails. Adding a test that causes index
creation failure by failing to deserialize entries.
commit 4a240dc27d77ae98dde10b4097e9eee6d515e1ce
Author: Dan Smith <up...@apache.org>
Date: 2016-12-15T01:08:36Z
GEODE-1272 Don't deserialize PDX objects when creating an index
Setting the flag to prevent deserialization of PDX objects while
populating an index that is defined on a partitioned region. We were
setting this flag in the member that initially created the index, but
not in other members that receive the IndexCreationMessage.
----
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by jhuynh1 <gi...@git.apache.org>.
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92691796
--- Diff: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java ---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+ Host host = Host.getHost(0);
+ VM vm0 = host.getVM(0);
+ VM vm1 = host.getVM(1);
+
+ SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+ cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+ };
+ vm0.invoke(createPR);
+ vm1.invoke(createPR);
+
+ // Do Puts. These objects can't be deserialized because they throw
+ // and exception from the constructor
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i, Integer.toString(i))));
+ });
+
+ // If this tries to deserialize the assets, it will fail
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex", "document", "/region");
--- End diff --
Yeah, that's the hardest part about the query tests, there are so many test classes that it can get hard to figure out where the best place for it is. If you feel ambitious/adventurous you can work on the hierarchy otherwise, I think this spot would be ok. Hopefully one day we can revisit and organize them a bit better.
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/318
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by jhuynh1 <gi...@git.apache.org>.
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92678100
--- Diff: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java ---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+ Host host = Host.getHost(0);
+ VM vm0 = host.getVM(0);
+ VM vm1 = host.getVM(1);
+
+ SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+ cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+ };
+ vm0.invoke(createPR);
+ vm1.invoke(createPR);
+
+ // Do Puts. These objects can't be deserialized because they throw
+ // and exception from the constructor
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i, Integer.toString(i))));
+ });
+
+ // If this tries to deserialize the assets, it will fail
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex", "document", "/region");
+ });
+
+ vm0.invoke(() -> {
+ QueryService qs = getCache().getQueryService();
+ SelectResults<Struct> results = (SelectResults) qs
+ .newQuery("<trace> select assetId,document from /region where document='B' limit 1000")
+ .execute();
+
+ assertEquals(3, results.size());
+ final Index index = qs.getIndex(getCache().getRegion("region"), "ContractDocumentIndex");
+ assertEquals(1, index.getStatistics().getTotalUses());
+ });
+ }
+
+ @Test
+ public void testFailureToCreateIndexOnRemoteNodeThrowsException() {
--- End diff --
Should there be a test for create index on local node fails but remotes are ok?
---
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] geode issue #318: Handle exceptions and don't deserialize PDX objects when c...
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on the issue:
https://github.com/apache/geode/pull/318
I added the additional tests we dicussed. Take a look and let me know what you think.
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by jhuynh1 <gi...@git.apache.org>.
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92677783
--- Diff: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java ---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+ Host host = Host.getHost(0);
+ VM vm0 = host.getVM(0);
+ VM vm1 = host.getVM(1);
+
+ SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+ cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+ };
+ vm0.invoke(createPR);
+ vm1.invoke(createPR);
+
+ // Do Puts. These objects can't be deserialized because they throw
+ // and exception from the constructor
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i, Integer.toString(i))));
+ });
+
+ // If this tries to deserialize the assets, it will fail
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex", "document", "/region");
--- End diff --
Would you be able to test out a range index (not a compact range index or hash)
This would require the index expression to contain multiple iterators such as /region r, r.someCollection p (assuming the values in the region have someCollection populated) The indexed expression itself could stay the same but it would try to create tuples. In this case, I would think that we would have to deserialize the value based on how that index maintains it's keys... Just wanted to make sure that index worked correctly (the test would obviously throw an error, but I just wanted to make sure that index still gets created correctly...)
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92688373
--- Diff: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionQueryDUnitTest.java ---
@@ -149,6 +154,92 @@ public void run() {
});
}
+ @Test
+ public void testIndexDoesNotDeserializePdxObjects() {
+ Host host = Host.getHost(0);
+ VM vm0 = host.getVM(0);
+ VM vm1 = host.getVM(1);
+
+ SerializableRunnableIF createPR = () -> {
+ Cache cache = getCache();
+ PartitionAttributesFactory paf = new PartitionAttributesFactory();
+ paf.setTotalNumBuckets(10);
+ cache.createRegionFactory(RegionShortcut.PARTITION).setPartitionAttributes(paf.create())
+ .create("region");
+ };
+ vm0.invoke(createPR);
+ vm1.invoke(createPR);
+
+ // Do Puts. These objects can't be deserialized because they throw
+ // and exception from the constructor
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ Region region = cache.getRegion("region");
+ region.put(0, new PdxNotDeserializableAsset(0, "B"));
+ region.put(10, new PdxNotDeserializableAsset(1, "B"));
+ region.put(1, new PdxNotDeserializableAsset(1, "B"));
+ IntStream.range(11, 100)
+ .forEach(i -> region.put(i, new PdxNotDeserializableAsset(i, Integer.toString(i))));
+ });
+
+ // If this tries to deserialize the assets, it will fail
+ vm0.invoke(() -> {
+ Cache cache = getCache();
+ cache.getQueryService().createHashIndex("ContractDocumentIndex", "document", "/region");
+ });
+
+ vm0.invoke(() -> {
+ QueryService qs = getCache().getQueryService();
+ SelectResults<Struct> results = (SelectResults) qs
+ .newQuery("<trace> select assetId,document from /region where document='B' limit 1000")
+ .execute();
+
+ assertEquals(3, results.size());
+ final Index index = qs.getIndex(getCache().getRegion("region"), "ContractDocumentIndex");
+ assertEquals(1, index.getStatistics().getTotalUses());
+ });
+ }
+
+ @Test
+ public void testFailureToCreateIndexOnRemoteNodeThrowsException() {
--- End diff --
Sure, I can add one.
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92688937
--- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java ---
@@ -8751,18 +8751,26 @@ public Index createIndex(boolean remotelyOriginated, IndexType indexType, String
// Second step is iterating over REs and populating all the created indexes
if (unpopulatedIndexes != null && unpopulatedIndexes.size() > 0) {
- throwException = populateEmptyIndexes(unpopulatedIndexes, exceptionsMap);
+ throwException |= populateEmptyIndexes(unpopulatedIndexes, exceptionsMap);
}
// Third step is to send the message to remote nodes
// Locally originated create index request.
// Send create request to other PR nodes.
- throwException =
+ throwException |=
sendCreateIndexesMessage(remotelyOriginated, indexDefinitions, indexes, exceptionsMap);
// If exception is throw in any of the above steps
if (throwException) {
- throw new MultiIndexCreationException(exceptionsMap);
+ try {
+ for (String indexName : exceptionsMap.keySet()) {
+ Index index = indexManager.getIndex(indexName);
+ indexManager.removeIndex(index);
+ removeIndex(index, remotelyOriginated);
--- End diff --
That's right. In my test, once I fixed the fact that we were losing the throwException flag, it didn't actually use the index, but the index was still there if you actually called getIndex.
---
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] geode pull request #318: Handle exceptions and don't deserialize PDX objects...
Posted by jhuynh1 <gi...@git.apache.org>.
Github user jhuynh1 commented on a diff in the pull request:
https://github.com/apache/geode/pull/318#discussion_r92677109
--- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java ---
@@ -8751,18 +8751,26 @@ public Index createIndex(boolean remotelyOriginated, IndexType indexType, String
// Second step is iterating over REs and populating all the created indexes
if (unpopulatedIndexes != null && unpopulatedIndexes.size() > 0) {
- throwException = populateEmptyIndexes(unpopulatedIndexes, exceptionsMap);
+ throwException |= populateEmptyIndexes(unpopulatedIndexes, exceptionsMap);
}
// Third step is to send the message to remote nodes
// Locally originated create index request.
// Send create request to other PR nodes.
- throwException =
+ throwException |=
sendCreateIndexesMessage(remotelyOriginated, indexDefinitions, indexes, exceptionsMap);
// If exception is throw in any of the above steps
if (throwException) {
- throw new MultiIndexCreationException(exceptionsMap);
+ try {
+ for (String indexName : exceptionsMap.keySet()) {
+ Index index = indexManager.getIndex(indexName);
+ indexManager.removeIndex(index);
+ removeIndex(index, remotelyOriginated);
--- End diff --
Just a question, was the calling method not cleaning up indexes if an exception was thrown?
---
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.
---