You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/09/29 18:59:31 UTC
[GitHub] [geode] jchen21 opened a new pull request #6919: Serialization filters
jchen21 opened a new pull request #6919:
URL: https://github.com/apache/geode/pull/6919
<!-- 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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] 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, check Concourse 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.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6919:
URL: https://github.com/apache/geode/pull/6919#discussion_r720583534
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/io/ServerJdkGlobalSerialFilterIntegrationTest.java
##########
@@ -64,6 +65,8 @@ public void setUp() throws IOException {
}
@Test
+ @Ignore
+ // jdk.serialFilter does not apply to ServerLauncher
Review comment:
This test may not be finished.
ServerLauncher should NOT configure global serial filter on Java 8 or Java 9.
Server JVM should however configure the jdk serial filter if it invokes `ManagementAgent.startAgent()`. The jdk serial filter work is fully implemented and merged to develop, but some of that code has been refactored so that the jdk serial filter and global serial filter share some classes and use similar internal APIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6919:
URL: https://github.com/apache/geode/pull/6919#discussion_r720584645
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/io/LocatorSerializableObjectFilterIntegrationTest.java
##########
@@ -195,7 +195,7 @@ public void configuresValidateSerializableObjects_onJava8() {
.isTrue();
assertThat(isValidateSerializableObjectsConfigured())
.as(VALIDATE_SERIALIZABLE_OBJECTS)
- .isTrue();
+ .isTrue(); // ??? why not false
Review comment:
It looks like `assumeThat(isJavaVersionAtLeast(JAVA_9)).isTrue();` on the first line of this test (`configuresValidateSerializableObjects_onJava8`) is wrong... it should be `isJavaVersionAtMost(JAVA_8)`.
Any tests that result in setting the global serial filter in a VM will need to either bounce that VM after the test or live in its own test class so that the JVM does away.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/io/LocatorSerializableObjectFilterIntegrationTest.java
##########
@@ -195,7 +195,7 @@ public void configuresValidateSerializableObjects_onJava8() {
.isTrue();
assertThat(isValidateSerializableObjectsConfigured())
.as(VALIDATE_SERIALIZABLE_OBJECTS)
- .isTrue();
+ .isTrue(); // ??? why not false
Review comment:
`LocatorLauncher` should configure BOTH global serial filter AND `validate-serializable-objects` when `start` is invoked and the JVM is Java 8.
I bet my code doesn't yet configure `validate-serializable-objects`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 closed pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
jchen21 closed pull request #6919:
URL: https://github.com/apache/geode/pull/6919
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6919:
URL: https://github.com/apache/geode/pull/6919#discussion_r723673464
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/SanctionedSerializablesFilterPattern.java
##########
@@ -108,6 +108,22 @@ private static StringJoiner dependenciesPattern() {
.add("sun.security.provider.certpath.SunCertPathBuilderException")
// geode-modules
- .add("org.apache.geode.modules.util.SessionCustomExpiry");
+ .add("org.apache.geode.modules.util.SessionCustomExpiry")
+ // geode-dunit
+ .add("org.apache.geode.management.JmxLocatorReconnectDistributedTest*")
+ .add("org.apache.geode.management.JmxServerReconnectDistributedTest*")
+ .add("org.apache.geode.management.internal.rest.ManagementRequestLoggingDistributedTest*")
+ .add("org.apache.geode.internal.metrics.MeterSubregistryReconnectDistributedTest*")
+ .add("org.apache.geode.internal.io.LocatorLauncherGlobalGlobalSerialFilterDistributedTest*")
+ .add("org.apache.geode.internal.cache.CompactOfflineDiskStoreDUnitTest*")
+ .add("org.apache.geode.internal.cache.ParallelDiskStoreRecoveryDUnitTest*")
+ .add("org.apache.geode.internal.cache.PartitionedRegionSingleHopDUnitTest*")
+ .add("org.apache.geode.internal.cache.BucketServerLocation66")
+ .add("org.apache.geode.internal.cache.ValidateOfflineDiskStoreDUnitTest*")
+ .add("org.apache.geode.logging.internal.LoggingWithReconnectDistributedTest*")
+ .add("org.apache.geode.distributed.HostedLocatorsDUnitTest*")
+ .add("org.apache.geode.management.internal.cli.commands.ShutdownCommandDUnitTest*")
+ .add("org.apache.geode.management.internal.cli.commands.ShutdownCommandOverHttpDUnitTest*")
+ .add("org.apache.geode.test.dunit.**");
Review comment:
Addressed by setting system properties `jdk.serialFilter` to `*`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6919:
URL: https://github.com/apache/geode/pull/6919#discussion_r723702449
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/io/ServerJdkGlobalSerialFilterIntegrationTest.java
##########
@@ -64,6 +65,8 @@ public void setUp() throws IOException {
}
@Test
+ @Ignore
+ // jdk.serialFilter does not apply to ServerLauncher
Review comment:
`ServerJdkGlobalSerialFilterIntegrationTest.java` is renamed to `ServerGlobalSerialFilterIntegrationTest.java` (without the string `jdk`).
Then it is split into two files, one file `ServerGlobalSerialFilterIntegrationTest.java` without setting system property `jdk.serialFilter` in its tests. The other file `ServerSetSystemPropertyGlobalSerialFilterIntegrationTestset` sets the system property `jdk.serialFilter` for each test method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6919:
URL: https://github.com/apache/geode/pull/6919#discussion_r720571017
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/SanctionedSerializablesFilterPattern.java
##########
@@ -108,6 +108,22 @@ private static StringJoiner dependenciesPattern() {
.add("sun.security.provider.certpath.SunCertPathBuilderException")
// geode-modules
- .add("org.apache.geode.modules.util.SessionCustomExpiry");
+ .add("org.apache.geode.modules.util.SessionCustomExpiry")
+ // geode-dunit
+ .add("org.apache.geode.management.JmxLocatorReconnectDistributedTest*")
+ .add("org.apache.geode.management.JmxServerReconnectDistributedTest*")
+ .add("org.apache.geode.management.internal.rest.ManagementRequestLoggingDistributedTest*")
+ .add("org.apache.geode.internal.metrics.MeterSubregistryReconnectDistributedTest*")
+ .add("org.apache.geode.internal.io.LocatorLauncherGlobalGlobalSerialFilterDistributedTest*")
+ .add("org.apache.geode.internal.cache.CompactOfflineDiskStoreDUnitTest*")
+ .add("org.apache.geode.internal.cache.ParallelDiskStoreRecoveryDUnitTest*")
+ .add("org.apache.geode.internal.cache.PartitionedRegionSingleHopDUnitTest*")
+ .add("org.apache.geode.internal.cache.BucketServerLocation66")
+ .add("org.apache.geode.internal.cache.ValidateOfflineDiskStoreDUnitTest*")
+ .add("org.apache.geode.logging.internal.LoggingWithReconnectDistributedTest*")
+ .add("org.apache.geode.distributed.HostedLocatorsDUnitTest*")
+ .add("org.apache.geode.management.internal.cli.commands.ShutdownCommandDUnitTest*")
+ .add("org.apache.geode.management.internal.cli.commands.ShutdownCommandOverHttpDUnitTest*")
+ .add("org.apache.geode.test.dunit.**");
Review comment:
We need to figure out a different solution for this. We shouldn't add dunit tests to the actual product pattern for sanctioned serializables.
I suspect that these tests may be starting a LocatorLauncher which then rejects deserializing classes in geode-core/src/distributedTest.
Tests such as `CompactOfflineDiskStoreDUnitTest` should instead do one of the following:
1. set the global serial filter to `"*"` in the VM that invokes `LocatorLauncher.start`
2. change to setting a global serial filter by passing -Djdk.serialFilter to a new Locator JVM from StartLocatorCommand instead of configuring it from LocatorLauncher.start
3. add an undocumented System Property to skip the configuring of serial filter in LocatorLauncher.start and set that in ChildVM.main
4. use `serializable-object-filter` to add the test and all dunit rules as user classes to the Locator's global serial filter (which I think we currently have commented out in LocatorLauncher)
I think [1] is the better option. We have plenty of tests so we don't really need these random dunit tests doing anything with the global serial filter. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #6919: Serialization filters
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6919:
URL: https://github.com/apache/geode/pull/6919#discussion_r723673464
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/SanctionedSerializablesFilterPattern.java
##########
@@ -108,6 +108,22 @@ private static StringJoiner dependenciesPattern() {
.add("sun.security.provider.certpath.SunCertPathBuilderException")
// geode-modules
- .add("org.apache.geode.modules.util.SessionCustomExpiry");
+ .add("org.apache.geode.modules.util.SessionCustomExpiry")
+ // geode-dunit
+ .add("org.apache.geode.management.JmxLocatorReconnectDistributedTest*")
+ .add("org.apache.geode.management.JmxServerReconnectDistributedTest*")
+ .add("org.apache.geode.management.internal.rest.ManagementRequestLoggingDistributedTest*")
+ .add("org.apache.geode.internal.metrics.MeterSubregistryReconnectDistributedTest*")
+ .add("org.apache.geode.internal.io.LocatorLauncherGlobalGlobalSerialFilterDistributedTest*")
+ .add("org.apache.geode.internal.cache.CompactOfflineDiskStoreDUnitTest*")
+ .add("org.apache.geode.internal.cache.ParallelDiskStoreRecoveryDUnitTest*")
+ .add("org.apache.geode.internal.cache.PartitionedRegionSingleHopDUnitTest*")
+ .add("org.apache.geode.internal.cache.BucketServerLocation66")
+ .add("org.apache.geode.internal.cache.ValidateOfflineDiskStoreDUnitTest*")
+ .add("org.apache.geode.logging.internal.LoggingWithReconnectDistributedTest*")
+ .add("org.apache.geode.distributed.HostedLocatorsDUnitTest*")
+ .add("org.apache.geode.management.internal.cli.commands.ShutdownCommandDUnitTest*")
+ .add("org.apache.geode.management.internal.cli.commands.ShutdownCommandOverHttpDUnitTest*")
+ .add("org.apache.geode.test.dunit.**");
Review comment:
Addressed by setting system properties `jdk.serialFilter` to `*`.
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/io/ServerJdkGlobalSerialFilterIntegrationTest.java
##########
@@ -64,6 +65,8 @@ public void setUp() throws IOException {
}
@Test
+ @Ignore
+ // jdk.serialFilter does not apply to ServerLauncher
Review comment:
`ServerJdkGlobalSerialFilterIntegrationTest.java` is renamed to `ServerGlobalSerialFilterIntegrationTest.java` (without the string `jdk`).
Then it is split into two files, one file `ServerGlobalSerialFilterIntegrationTest.java` without setting system property `jdk.serialFilter` in its tests. The other file `ServerSetSystemPropertyGlobalSerialFilterIntegrationTestset` sets the system property `jdk.serialFilter` for each test method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org