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/17 21:13:45 UTC
[GitHub] [geode] kirklund opened a new pull request #6878: GEODE-9486: Improve AnalyzeSerializables integration tests
kirklund opened a new pull request #6878:
URL: https://github.com/apache/geode/pull/6878
Improve debugging information for AnalyzeSerializables integration
tests:
- Provide new failure message when no SanctionedSerializablesService
exists.
- Create ANALYZE_SERIALIZABLES.md to provide detailed instructions for
failures involving AnalyzeSerializables integration tests.
- Reference ANALYZE_SERIALIZABLES.md in failure messages.
Remove geode-serialization and geode-common jars from geode-pulse
.war file:
- Change getModuleClass() to return Optional.
- Delete PulseSanctionedSerializablesService and its resources.
- Change geode-pulse dependency on geode-serialization to be for
integrationTest only.
<!-- 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] gesterzhou commented on a change in pull request #6878: GEODE-9486: Improve AnalyzeSerializables integration tests
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6878:
URL: https://github.com/apache/geode/pull/6878#discussion_r712514481
##########
File path: geode-serialization/ANALYZE_SERIALIZABLES.md
##########
@@ -0,0 +1,116 @@
+# Analyze Serializables
+
+## Adding AnalyzeSerializables test to a new geode module
+
+If you've created a new geode module, then add an integration test to detect any additions or changes to serializable classes in the module.
+
+Change the module's `build.gradle` to add a dependency on geode-serialization:
+```
+integrationTestImplementation(project(':geode-serialization'))
+```
+
+Create `AnalyzeModuleSerializablesIntegrationTest` that extends `AnalyzeSerializablesJUnitTestBase`. It needs to be in package `org.apache.geode.codeAnalysis`.
+
+## Implementing SanctionedSerializablesService
+
+If you've changed or added any serializable classes to a geode module, the previously added integration test should fail. You'll need to implement `SanctionedSerializablesService` for the geode module.
+
+Change the module's `build.gradle` to add a dependency on geode-serialization:
+```
+implementation(project(':geode-serialization'))
+```
+
+Create a new `ModuleSanctionedSerializablesService` that implements `SanctionedSerializablesService`:
+```
+geode-module/src/main/java/org/apache/geode/module/internal/ModuleSanctionedSerializablesService.java
+```
+
+Add a service file for `SanctionedSerializablesService`:
+```
+geode-module/src/main/resources/META-INF/services/org.apache.geode.internal.serialization.SanctionedSerializablesService
+```
+
+Add a line to the service file specifying the fully qualified name of the service implementation:
+```
+org.apache.geode.module.internal.ModuleSanctionedSerializablesService
+```
+
+Update `AnalyzeModuleSerializablesIntegrationTest` to return the new `SanctionedSerializablesService` implementation from `getModuleClass`:
+```java
+@Override
+protected Optional<Class<?>> getModuleClass() {
+ return Optional.of(ModuleSanctionedSerializablesService.class);
+}
+```
+
+## Fixing failures in AnalyzeModuleSerializablesIntegrationTest
+
+`AnalyzeModuleSerializablesIntegrationTest` analyzes serializable classes in a module. It fails if either:
+- a new serializable class is added to the main src
+- an existing serializable class in main src is modified
+
+The content of the failure message depends on whether the module implements `SanctionedSerializablesService`.
+
+If it implements `SanctionedSerializablesService`, the failure message looks like:
+```
+New or moved classes----------------------------------------
+org/apache/geode/CancelException,true,3215578659523282642
+
+If the class is not persisted or sent over the wire, add it to the file
+ /path/to/geode/geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
+Otherwise, if this doesn't break backward compatibility, copy the file
Review comment:
"doesn't"?
I wonder if it's typo. Should it be "does" here?
--
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 #6878: GEODE-9486: Improve AnalyzeSerializables integration tests
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6878:
URL: https://github.com/apache/geode/pull/6878#discussion_r711369453
##########
File path: geode-serialization/ANALYZE_SERIALIZABLES.md
##########
@@ -0,0 +1,129 @@
+# Analyze Serializables
+
+## Adding AnalyzeSerializables test to a new geode module
+
+If you've created a new geode module, then add an integration test to detect any additions or
+changes to serializable classes in the module.
+
+Change the module's build.gradle to add a dependency on geode-serialization:
+```
+integrationTestImplementation(project(':geode-serialization'))
+```
+
+Create `AnalyzeModuleSerializablesIntegrationTest` that extends
+`AnalyzeSerializablesJUnitTestBase`. It needs to be in package `org.apache.geode.codeAnalysis`.
+
+## Implementing SanctionedSerializablesService
+
+If you've changed or added any serializable classes to a geode module, the previously added
+integration test should fail. You'll need to implement SanctionedSerializablesService for the geode
+module.
+
+Change the module's build.gradle to add a dependency on geode-serialization:
+```
+implementation(project(':geode-serialization'))
+```
+
+Create a new ModuleSanctionedSerializablesService that implements SanctionedSerializablesService:
+```
+src/main/java/org/apache/geode/module/internal/ModuleSanctionedSerializablesService.java
+```
+
+Add a service file for SanctionedSerializablesService:
+```
+src/main/resources/META-INF/services/org.apache.geode.internal.serialization.SanctionedSerializablesService
+```
+
+Add a line to the service file specifying the fully qualified name of the service implementation:
+```
+org.apache.geode.module.internal.ModuleSanctionedSerializablesService
+```
+
+Update AnalyzeModuleSerializablesIntegrationTest to return the new SanctionedSerializablesService
+implementation from `getModuleClass`:
+```java
+@Override
+protected Optional<Class<?>> getModuleClass() {
+ return Optional.of(ModuleSanctionedSerializablesService.class);
+}
+```
+
+## Fixing failures in AnalyzeModuleSerializablesIntegrationTest
+
+AnalyzeModuleSerializablesIntegrationTest analyzes serializable classes in a module. It fails if
+either:
+- a new serializable class is added to the main src
+- an existing serializable class in main src is modified
+
+The content of the failure message depends on whether the module implements
+SanctionedSerializablesService.
+
+If it implements SanctionedSerializablesService, the failure message looks like:
+```
+New or moved classes----------------------------------------
+org/apache/geode/CancelException,true,3215578659523282642
+
+If the class is not persisted or sent over the wire, add it to the file
+ /Users/klund/dev/geode3/geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
Review comment:
No, I'll remove it
--
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] demery-pivotal commented on a change in pull request #6878: GEODE-9486: Improve AnalyzeSerializables integration tests
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #6878:
URL: https://github.com/apache/geode/pull/6878#discussion_r711368370
##########
File path: geode-serialization/ANALYZE_SERIALIZABLES.md
##########
@@ -0,0 +1,129 @@
+# Analyze Serializables
+
+## Adding AnalyzeSerializables test to a new geode module
+
+If you've created a new geode module, then add an integration test to detect any additions or
+changes to serializable classes in the module.
+
+Change the module's build.gradle to add a dependency on geode-serialization:
+```
+integrationTestImplementation(project(':geode-serialization'))
+```
+
+Create `AnalyzeModuleSerializablesIntegrationTest` that extends
+`AnalyzeSerializablesJUnitTestBase`. It needs to be in package `org.apache.geode.codeAnalysis`.
+
+## Implementing SanctionedSerializablesService
+
+If you've changed or added any serializable classes to a geode module, the previously added
+integration test should fail. You'll need to implement SanctionedSerializablesService for the geode
+module.
+
+Change the module's build.gradle to add a dependency on geode-serialization:
+```
+implementation(project(':geode-serialization'))
+```
+
+Create a new ModuleSanctionedSerializablesService that implements SanctionedSerializablesService:
+```
+src/main/java/org/apache/geode/module/internal/ModuleSanctionedSerializablesService.java
+```
+
+Add a service file for SanctionedSerializablesService:
+```
+src/main/resources/META-INF/services/org.apache.geode.internal.serialization.SanctionedSerializablesService
+```
+
+Add a line to the service file specifying the fully qualified name of the service implementation:
+```
+org.apache.geode.module.internal.ModuleSanctionedSerializablesService
+```
+
+Update AnalyzeModuleSerializablesIntegrationTest to return the new SanctionedSerializablesService
+implementation from `getModuleClass`:
+```java
+@Override
+protected Optional<Class<?>> getModuleClass() {
+ return Optional.of(ModuleSanctionedSerializablesService.class);
+}
+```
+
+## Fixing failures in AnalyzeModuleSerializablesIntegrationTest
+
+AnalyzeModuleSerializablesIntegrationTest analyzes serializable classes in a module. It fails if
+either:
+- a new serializable class is added to the main src
+- an existing serializable class in main src is modified
+
+The content of the failure message depends on whether the module implements
+SanctionedSerializablesService.
+
+If it implements SanctionedSerializablesService, the failure message looks like:
+```
+New or moved classes----------------------------------------
+org/apache/geode/CancelException,true,3215578659523282642
+
+If the class is not persisted or sent over the wire, add it to the file
+ /Users/klund/dev/geode3/geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
+Otherwise, if this doesn't break backward compatibility, copy the file
+ /Users/klund/dev/geode3/geode-core/build/integrationTest/test-worker-000009/actualSerializables.dat
+ to
+ /Users/klund/dev/geode3/geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt
+If this potentially breaks backward compatibility, follow the instructions in
+ README.MD
Review comment:
Change `README.MD` to the new path to the instructions.
##########
File path: geode-serialization/ANALYZE_SERIALIZABLES.md
##########
@@ -0,0 +1,129 @@
+# Analyze Serializables
+
+## Adding AnalyzeSerializables test to a new geode module
+
+If you've created a new geode module, then add an integration test to detect any additions or
+changes to serializable classes in the module.
+
+Change the module's build.gradle to add a dependency on geode-serialization:
+```
+integrationTestImplementation(project(':geode-serialization'))
+```
+
+Create `AnalyzeModuleSerializablesIntegrationTest` that extends
+`AnalyzeSerializablesJUnitTestBase`. It needs to be in package `org.apache.geode.codeAnalysis`.
+
+## Implementing SanctionedSerializablesService
+
+If you've changed or added any serializable classes to a geode module, the previously added
+integration test should fail. You'll need to implement SanctionedSerializablesService for the geode
+module.
+
+Change the module's build.gradle to add a dependency on geode-serialization:
+```
+implementation(project(':geode-serialization'))
+```
+
+Create a new ModuleSanctionedSerializablesService that implements SanctionedSerializablesService:
+```
+src/main/java/org/apache/geode/module/internal/ModuleSanctionedSerializablesService.java
+```
+
+Add a service file for SanctionedSerializablesService:
+```
+src/main/resources/META-INF/services/org.apache.geode.internal.serialization.SanctionedSerializablesService
+```
+
+Add a line to the service file specifying the fully qualified name of the service implementation:
+```
+org.apache.geode.module.internal.ModuleSanctionedSerializablesService
+```
+
+Update AnalyzeModuleSerializablesIntegrationTest to return the new SanctionedSerializablesService
+implementation from `getModuleClass`:
+```java
+@Override
+protected Optional<Class<?>> getModuleClass() {
+ return Optional.of(ModuleSanctionedSerializablesService.class);
+}
+```
+
+## Fixing failures in AnalyzeModuleSerializablesIntegrationTest
+
+AnalyzeModuleSerializablesIntegrationTest analyzes serializable classes in a module. It fails if
+either:
+- a new serializable class is added to the main src
+- an existing serializable class in main src is modified
+
+The content of the failure message depends on whether the module implements
+SanctionedSerializablesService.
+
+If it implements SanctionedSerializablesService, the failure message looks like:
+```
+New or moved classes----------------------------------------
+org/apache/geode/CancelException,true,3215578659523282642
+
+If the class is not persisted or sent over the wire, add it to the file
+ /Users/klund/dev/geode3/geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/excludedClasses.txt
Review comment:
Do you want your username immortalized in these file paths?
--
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 merged pull request #6878: GEODE-9486: Improve AnalyzeSerializables integration tests
Posted by GitBox <gi...@apache.org>.
kirklund merged pull request #6878:
URL: https://github.com/apache/geode/pull/6878
--
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 pull request #6878: GEODE-9486: Improve AnalyzeSerializables integration tests
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #6878:
URL: https://github.com/apache/geode/pull/6878#issuecomment-923193612
I recommend putting a link to `ANALYZE_SERIALIZABLES.md` in one of the `md` files in the Geode root directory, where you see appropriate. So that it is easy to find the new instructions you added. Otherwise, people might not be aware of the existence of this helpful doc.
--
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