Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
Geode-core provides suitable public alternatives, I'm sure Spring Data Geode would be able to adapt and use the certified public API's. --Udo On 11/26/18 15:48, Dan Smith wrote: Our wire format and disk format generally does *not* depend on class names or packages. Internal classes implement DataSerializableFixedID and are sent using a fixed integer ID. So we should be able to move these classes around without affecting the on the wire format. I don't think we should shy away from changing *internal* packages. Worst case is that we break someone's code and encourage them to ask for a public API, which is a conversation we should be having anyway. But to allay some concerns I took a look at what internal classes spring data geode is using (on the master branch). I think all of these are in geode-core. So if we leave the packages in geode-core alone, SDG will be fine. import org.apache.geode.cache.query.internal.ResultsBag; import org.apache.geode.distributed.internal.DistributionConfig; import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.distributed.internal.InternalLocator; import org.apache.geode.internal.DistributionLocator; import org.apache.geode.internal.GemFireVersion; import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.cache.GemFireCacheImpl; import org.apache.geode.internal.cache.LocalRegion; import org.apache.geode.internal.cache.PartitionedRegion; import org.apache.geode.internal.cache.PoolManagerImpl; import org.apache.geode.internal.cache.UserSpecifiedRegionAttributes; import org.apache.geode.internal.concurrent.ConcurrentHashSet; import org.apache.geode.internal.datasource.ConfigProperty; import org.apache.geode.internal.datasource.GemFireBasicDataSource; import org.apache.geode.internal.jndi.JNDIInvoker; import org.apache.geode.internal.security.shiro.GeodePermissionResolver; import org.apache.geode.management.internal.cli.domain.RegionInformation; import org.apache.geode.management.internal.cli.functions.GetRegionsFunction; import org.apache.geode.management.internal.security.ResourcePermissions; import org.apache.geode.pdx.internal.PdxInstanceEnum; import org.apache.geode.pdx.internal.PdxInstanceFactoryImpl; On Mon, Nov 26, 2018 at 3:24 PM Kirk Lund wrote: One problem about moving around internal classes is that Geode uses (proprietary and Java-based) serialization on the wire instead of defining a wire-format that isn't dependent on class names/structures/packages. I for one would love to move to a real wire-format with a well-defined protocol but that's probably a separate project in its own right. Are you really convinced that we could move internal classes around without breaking rolling upgrades, client-server backwards compatibility and diskstore contents? On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: I think we can actually fix most of these issues without geode-2.0. Most of these are in internal packages, which means we can change the package of these classes without breaking users. The only concerning one is org.apache.geode.cache.util, which is a public package. However, the AutoBalancer is actually still marked @Experimental, so we could technically move that as well. Or maybe deprecate the old one and it's associated jar, and create a new jar with a reasonable package. JDK11 users could just avoid the old jar. I have been wondering for a while if we should just fold geode-cq and geode-wan back into geode-core. These are not really properly separated out, they are very tangled with core. The above package overlap kinda shows that as well. But maybe going through the effort of renaming the packages to make them java 11 modules would help improve the code anyway! -Dan On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols wrote: To package Geode as Java 11 Jigsaw module(s), a major hurdle is the requirement that packages cannot be split across modules. If we simply map existing Geode jars to Modules, we have about 10 split packages (see table below). Any restructuring would potentially have to wait until Geode 2.0. A workaround would be to map existing packages into a small number of new modules (e.g. geode-api and geode-internal). Existing jars would remain as-is. Users making the transition to Java 11 /w Jigsaw already need to switch from CLASSPATH to MODULEPATH so the inconvenience of a different naming scheme for Geode-as-modules might be acceptable (however, once module naming and organization is established, we may be stuck with it for a long time). Thoughts? Is it even possible to rearrange all classes in each package below into a single jar? Is doing so desirable enough to defer Java 11 support until a yet-unplanned Geode 2.0, or perhaps to drive such a release? Or, is it satisfactory to fatten Geode releases to include one set of jars for CLASSPATH use, plus a different set of jars for MODULEPATH use? Package Jar
SampleHandler interface and handling of statistics samples
SampleHandler is in the org.apache.geode.internal.statistics package. The SampleHandler interface was originally introduced to allow multiple handlers to receive notification of a statistics sample. Before this interface, the StatSampler used a StatArchiveWriter directly to write to the .gfs (Geode Statistics archive file). The StatSampler takes a sample of all statistics (every second by default) and then invokes every registered SampleHandler. The interface defines 4 methods: *1) sampled* -- invoked whenever there's a sample taken -- provides a List of ResourceInstances with latest values *2) allocatedResourceType* -- invoked when a new ResourceType has been defined -- this may be caused by enabling a feature or adding a new instance of a feature or the User may have used the Statistics API to define a new Statistics ResourceType *3) allocatedResourceInstance* -- invoked when a new ResourceInstance of a ResourceType is created *4) destroyedResourceInstance* -- invoked when a ResourceInstance is destroyed A ResourceType is basically a definition of a bucket of key/value stats. Within Geode, each ResourceType is typically a class such as DistributionStats. A ResourceInstance is an instance of that with actual values. One ResourceType may have many ResourceInstances. The format of a .gfs file closely follows those 4 methods. ResourceType definition is written to the .gfs file before any sample can contain a ResourceInstance of that type. A ResourceInstance allocated record is also written to the .gfs file before any sample may contain any values for that instance. A ResourceInstance destroyed record is written indicating the end of lifecycle for a particular instance. There are two main implementations of SampleHandler: *A) StatArchiveHandler* -- mechanism that writes binary data to the .gfs (Geode Statistics File) *B) StatMonitorHandler* -- mechanism that notifies objects that are clients to the statistics monitoring API (which is what updates the metrics attributes on Geode mbeans The purpose of this design is to allow a developer to build a new implementation of SampleHandler that writes out to another format or defines some sort of custom consumer of Statistics samples. For example, you could define a MicrometerHandler which adapts Geode stats into Micrometer, or you could define an entirely new custom file format of stats. By default, the StatSampler samples the statistics every second and there is only one StatSampler thread, so stat sampling is sensitive to the performance of any implementation of SampleHandler. For example, you might not want to define a handler that writes to a RDB over the network. Performance problems in a SampleHandler could cause gaps in stats values, delayed updating of Geode mbeans for monitoring tools, and it could cause someone who is analyzing Geode artifacts to think that a stop-the-world GC pause has occurred -- the time between stat samples is generally assumed to be fairly consistent unless a serious GC pause occurs.
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
Our wire format and disk format generally does *not* depend on class names or packages. Internal classes implement DataSerializableFixedID and are sent using a fixed integer ID. So we should be able to move these classes around without affecting the on the wire format. I don't think we should shy away from changing *internal* packages. Worst case is that we break someone's code and encourage them to ask for a public API, which is a conversation we should be having anyway. But to allay some concerns I took a look at what internal classes spring data geode is using (on the master branch). I think all of these are in geode-core. So if we leave the packages in geode-core alone, SDG will be fine. import org.apache.geode.cache.query.internal.ResultsBag; import org.apache.geode.distributed.internal.DistributionConfig; import org.apache.geode.distributed.internal.InternalDistributedSystem; import org.apache.geode.distributed.internal.InternalLocator; import org.apache.geode.internal.DistributionLocator; import org.apache.geode.internal.GemFireVersion; import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.cache.GemFireCacheImpl; import org.apache.geode.internal.cache.LocalRegion; import org.apache.geode.internal.cache.PartitionedRegion; import org.apache.geode.internal.cache.PoolManagerImpl; import org.apache.geode.internal.cache.UserSpecifiedRegionAttributes; import org.apache.geode.internal.concurrent.ConcurrentHashSet; import org.apache.geode.internal.datasource.ConfigProperty; import org.apache.geode.internal.datasource.GemFireBasicDataSource; import org.apache.geode.internal.jndi.JNDIInvoker; import org.apache.geode.internal.security.shiro.GeodePermissionResolver; import org.apache.geode.management.internal.cli.domain.RegionInformation; import org.apache.geode.management.internal.cli.functions.GetRegionsFunction; import org.apache.geode.management.internal.security.ResourcePermissions; import org.apache.geode.pdx.internal.PdxInstanceEnum; import org.apache.geode.pdx.internal.PdxInstanceFactoryImpl; On Mon, Nov 26, 2018 at 3:24 PM Kirk Lund wrote: > One problem about moving around internal classes is that Geode uses > (proprietary and Java-based) serialization on the wire instead of defining > a wire-format that isn't dependent on class names/structures/packages. > > I for one would love to move to a real wire-format with a well-defined > protocol but that's probably a separate project in its own right. > > Are you really convinced that we could move internal classes around without > breaking rolling upgrades, client-server backwards compatibility and > diskstore contents? > > On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: > > > I think we can actually fix most of these issues without geode-2.0. Most > of > > these are in internal packages, which means we can change the package of > > these classes without breaking users. The only concerning one is > > org.apache.geode.cache.util, which is a public package. However, the > > AutoBalancer is actually still marked @Experimental, so we could > > technically move that as well. Or maybe deprecate the old one and it's > > associated jar, and create a new jar with a reasonable package. JDK11 > users > > could just avoid the old jar. > > > > I have been wondering for a while if we should just fold geode-cq and > > geode-wan back into geode-core. These are not really properly separated > > out, they are very tangled with core. The above package overlap kinda > shows > > that as well. But maybe going through the effort of renaming the packages > > to make them java 11 modules would help improve the code anyway! > > > > -Dan > > > > > > > > > > On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols > wrote: > > > > > To package Geode as Java 11 Jigsaw module(s), a major hurdle is the > > > requirement that packages cannot be split across modules. > > > > > > If we simply map existing Geode jars to Modules, we have about 10 split > > > packages (see table below). Any restructuring would potentially have > to > > > wait until Geode 2.0. > > > > > > A workaround would be to map existing packages into a small number of > new > > > modules (e.g. geode-api and geode-internal). Existing jars would > remain > > > as-is. Users making the transition to Java 11 /w Jigsaw already need > to > > > switch from CLASSPATH to MODULEPATH so the inconvenience of a > different > > > naming scheme for Geode-as-modules might be acceptable (however, once > > > module naming and organization is established, we may be stuck with it > > for > > > a long time). > > > > > > Thoughts? > > > > > > Is it even possible to rearrange all classes in each package below > into a > > > single jar? Is doing so desirable enough to defer Java 11 support > until > > a > > > yet-unplanned Geode 2.0, or perhaps to drive such a release? > > > > > > Or, is it satisfactory to fatten Geode releases to include one set of > > jars > > > for CLASSPATH use, plus a different set of
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
One problem about moving around internal classes is that Geode uses (proprietary and Java-based) serialization on the wire instead of defining a wire-format that isn't dependent on class names/structures/packages. I for one would love to move to a real wire-format with a well-defined protocol but that's probably a separate project in its own right. Are you really convinced that we could move internal classes around without breaking rolling upgrades, client-server backwards compatibility and diskstore contents? On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: > I think we can actually fix most of these issues without geode-2.0. Most of > these are in internal packages, which means we can change the package of > these classes without breaking users. The only concerning one is > org.apache.geode.cache.util, which is a public package. However, the > AutoBalancer is actually still marked @Experimental, so we could > technically move that as well. Or maybe deprecate the old one and it's > associated jar, and create a new jar with a reasonable package. JDK11 users > could just avoid the old jar. > > I have been wondering for a while if we should just fold geode-cq and > geode-wan back into geode-core. These are not really properly separated > out, they are very tangled with core. The above package overlap kinda shows > that as well. But maybe going through the effort of renaming the packages > to make them java 11 modules would help improve the code anyway! > > -Dan > > > > > On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols wrote: > > > To package Geode as Java 11 Jigsaw module(s), a major hurdle is the > > requirement that packages cannot be split across modules. > > > > If we simply map existing Geode jars to Modules, we have about 10 split > > packages (see table below). Any restructuring would potentially have to > > wait until Geode 2.0. > > > > A workaround would be to map existing packages into a small number of new > > modules (e.g. geode-api and geode-internal). Existing jars would remain > > as-is. Users making the transition to Java 11 /w Jigsaw already need to > > switch from CLASSPATH to MODULEPATH so the inconvenience of a different > > naming scheme for Geode-as-modules might be acceptable (however, once > > module naming and organization is established, we may be stuck with it > for > > a long time). > > > > Thoughts? > > > > Is it even possible to rearrange all classes in each package below into a > > single jar? Is doing so desirable enough to defer Java 11 support until > a > > yet-unplanned Geode 2.0, or perhaps to drive such a release? > > > > Or, is it satisfactory to fatten Geode releases to include one set of > jars > > for CLASSPATH use, plus a different set of jars for MODULEPATH use? > > > > > > Package > > Jar > > org.apache.geode.cache.client.internal > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.client.internal.locator.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.query.internal.cq > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.cache.util > > geode-core-1.8.0.jar > > geode-rebalancer-1.8.0.jar > > org.apache.geode.internal > > geode-connectors-1.8.0.jar > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-lucene-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.tier.sockets.command > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.internal.cache.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.parallel > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.serial > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.protocol.protobuf.v1 > > geode-protobuf-1.8.0.jar > > geode-protobuf-messages-1.8.0.jar >
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
@Galen - If a method is added to a Java interface (e.g. CacheListener), then classes implementing the interface must define the method, or a compile error will occur. This is true in the case where 1) the class implements CacheListener directly rather than extending CacheListenerAdapter and 2) where the added method on CacheListener is not a default method. A few things/thoughts: 1) First, CLASSPATH does not go away, AFAIK. [1] 2) I am not intimately knowledgeable on (all things) JPMS yet, but a good interim solution might be to opt for filename-based, "automatic modules" in Geode. This perhaps does not solve the split package problem. But, as I previously mentioned, I think having "internal" APIs is a problem that needs to be solved before modularizing. 3) What percentage of users truly really care about JPMS yet? Java 9/10/11 deployments still pale in comparison to the number of Java 8 deployments and will for the foreseeable future, especially with Oracle's new release cadence and LTS policies, which many devs don't like. Food for thought, -j [1] https://blog.joda.org/2018/09/from-java-8-to-java-11.html On Mon, Nov 26, 2018 at 1:14 PM Jacob Barrett wrote: > Before we get to far off the intention of this discussion, let's refocus. > The purpose of the this discussion isn’t to find the “best” modules > solution but rather to find one that is “good enough” to allow Geode 1.x to > work in a Java 11 world with a reasonable user experience. While we can run > the server effectively in Java 11 we cannot consume modules nor be consumed > as modules by a modularized Java 11 application. Whatever steps we take in > Geode 1.x will likely be thrown away for a solid solution for 2.x. > > So the question is: > 1) Is refactoring internal to avoid the split packaging issue have a > significant negative impact on downstream projects that currently use > internal classes due to deficiencies in our public APIs? If so, what are > the impacts and can we minimize them in other ways? > > 2) Would an uber module be a safer and less negatively impactful solution > than option 1? > > 3) Is there some other interim solution nobody has thrown out yet? > > -Jake > > -- -John john.blum10101 (skype)
Re: Using SystemOutRule or SystemErrRule in Geode tests
The following unit test will pass repeatedly in both your IDE (ex: run until failure) or in stressNewTest: public class SystemOutRuleTest { @Rule public SystemOutRule systemOutRule = new SystemOutRule().enableLog(); @Test public void passesRepeatedly() { System.out.println("hello"); assertThat(systemOutRule.getLog()).contains("hello"); } } But the following test will fail after the first time the test is executed (ex: run until failure in IntelliJ) : public class SystemOutRuleTest { @Rule public SystemOutRule systemOutRule = new SystemOutRule().enableLog(); @Test public void passesFirstTime() { LogManager.getLogger().info("hello"); assertThat(systemOutRule.getLog()).contains("hello"); } } If you need to verify that a class is logging a specific message in some circumstances then you can extract that test from the Unit Test and move it to an Integration Test. You do *not* have to annotate SystemOutRule with @ClassRule but I recommend doing that especially if you're using other Rules doing some complicated setUp involving Geode classes: public class CacheLoggingIntegrationTest { private InternalCache cache; @Rule public SystemOutRule systemOutRule = new SystemOutRule().enableLog(); @Before public void setUp() { cache = (InternalCache) new CacheFactory().set(LOCATORS, "").create(); } @After public void tearDown() { cache.close(); } @Test public void passesFirstTime() { cache.getLogger().info("hello"); assertThat(systemOutRule.getLog()).contains("hello"); } } If you change the SystemOutRule in CacheLoggingIntegrationTest to a static @ClassRule: @ClassRule public static SystemOutRule systemOutRule = new SystemOutRule().enableLog(); ...then it will repeatedly pass if you run-until-failure in IntelliJ but only because IJ ends up running it as one test class. It will still fail when run repeatedly by stressNewTest or if a previous test class caused Log4J2 to startup within the same JVM. On Mon, Nov 26, 2018 at 1:46 PM Kirk Lund wrote: > Log4J and Logback both capture a reference to System.out and/or System.err > and then use that reference thereafter. This means that any manipulation of > System.out or System.err -- using System.setOut(PrintStream) or > System.setErr(PrintStream) -- probably isn't going to play nicely with > logging in general. > > Here's what that means for Geode tests... > > If you're writing a Unit Test, then that test will be executed in a > long-lived JVM in which other Unit Tests are also going to be executed. The > SystemOutRule from system-rules will work with code that uses System.out > directly but will not work repeatedly with loggers. > > If you're writing an Integration Test or Distributed Test, then that test > will be executed in a fresh JVM (fork every one is the config), so as long > as you can get the SystemOutRule before to invoke before logging, the test > will behave. In general that means you'll want to annotate it > with @BeforeClass. > > Here are a couple examples in which log4j2 captures System.out to > illustrate why SystemOutRule can't intercept the output after invoking > System.setOut(PrintStream): > > log4j-core/src/main/java/org/apache/logging/log4j/core/config/status/StatusConfiguration.java:43: > private static final PrintStream DEFAULT_STREAM = System.out; > > log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java:87: > ps = System.out; > > If SystemOutRule executes its before -- System.setOut(PrintStream) -- > after the above code, logging will be printing to a different PrintStream > than the PrintStream that SystemOutRule inserted into System. >
Using SystemOutRule or SystemErrRule in Geode tests
Log4J and Logback both capture a reference to System.out and/or System.err and then use that reference thereafter. This means that any manipulation of System.out or System.err -- using System.setOut(PrintStream) or System.setErr(PrintStream) -- probably isn't going to play nicely with logging in general. Here's what that means for Geode tests... If you're writing a Unit Test, then that test will be executed in a long-lived JVM in which other Unit Tests are also going to be executed. The SystemOutRule from system-rules will work with code that uses System.out directly but will not work repeatedly with loggers. If you're writing an Integration Test or Distributed Test, then that test will be executed in a fresh JVM (fork every one is the config), so as long as you can get the SystemOutRule before to invoke before logging, the test will behave. In general that means you'll want to annotate it with @BeforeClass. Here are a couple examples in which log4j2 captures System.out to illustrate why SystemOutRule can't intercept the output after invoking System.setOut(PrintStream): log4j-core/src/main/java/org/apache/logging/log4j/core/config/status/StatusConfiguration.java:43: private static final PrintStream DEFAULT_STREAM = System.out; log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java:87: ps = System.out; If SystemOutRule executes its before -- System.setOut(PrintStream) -- after the above code, logging will be printing to a different PrintStream than the PrintStream that SystemOutRule inserted into System.
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
Before we get to far off the intention of this discussion, let's refocus. The purpose of the this discussion isn’t to find the “best” modules solution but rather to find one that is “good enough” to allow Geode 1.x to work in a Java 11 world with a reasonable user experience. While we can run the server effectively in Java 11 we cannot consume modules nor be consumed as modules by a modularized Java 11 application. Whatever steps we take in Geode 1.x will likely be thrown away for a solid solution for 2.x. So the question is: 1) Is refactoring internal to avoid the split packaging issue have a significant negative impact on downstream projects that currently use internal classes due to deficiencies in our public APIs? If so, what are the impacts and can we minimize them in other ways? 2) Would an uber module be a safer and less negatively impactful solution than option 1? 3) Is there some other interim solution nobody has thrown out yet? -Jake
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
> > If you want to > maintain an API as internal, then you should properly protect that API with > the appropriate *Java* access modifiers (private, package-private and > protected). > Hopefully modules will allow us to do this better, by restricting which *classes* are visible outside the module. However, Java doesn't have a concept of submodules, which means there's no way to make org.apache.geode.cache.tier.sockets visible to org.apache.cache or org.apache.cache.tier without also being visible to everything. We can't make InternalCache (for example) only visible to our own code without declaring it package-private and putting (effectively) all of Geode in the same package. Adding a method to > an interface, is an API breaking change. > I don't agree with this. If a new method is added, old API works just fine, and unless I'm mistaken (and I may be), any code that used the old interface should be able to use the new interface without issues. Galen On Mon, Nov 26, 2018 at 12:13 PM John Blum wrote: > *> Most of these are in internal packages, which means we can change the > package of these classes without breaking users.* > > I don't agree with this. > > Some functionality in Geode required by an application, or framework, is > only available in an "internal" package, unfortunately. This has to do > with the fact that many interfaces in Geode were not well designed. > > In general, and as a general rule of thumb, anytime you change a method > name or method signature, it is an API breaking change. Adding a method to > an interface, is an API breaking change. Removing methods from an > interface is (potentially) an API breaking change. Changing a type is an > API breaking change. There are several other examples. Any class/method > (dare I say field of a class) that is not "properly protected" is part of > your API, regardless if we deem it be "internal" only or not, and > (possibly) will affect users. > > I think any level of modularity (and maturing) is pointless without 1) > first, having well defined contracts/interfaces between functional areas > (based on concerns) and 2) using appropriate access modifiers and cleanly > delineating APIs/SPIs from implementations/providers. > > -j > > > > On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: > > > I think we can actually fix most of these issues without geode-2.0. Most > of > > these are in internal packages, which means we can change the package of > > these classes without breaking users. The only concerning one is > > org.apache.geode.cache.util, which is a public package. However, the > > AutoBalancer is actually still marked @Experimental, so we could > > technically move that as well. Or maybe deprecate the old one and it's > > associated jar, and create a new jar with a reasonable package. JDK11 > users > > could just avoid the old jar. > > > > I have been wondering for a while if we should just fold geode-cq and > > geode-wan back into geode-core. These are not really properly separated > > out, they are very tangled with core. The above package overlap kinda > shows > > that as well. But maybe going through the effort of renaming the packages > > to make them java 11 modules would help improve the code anyway! > > > > -Dan > > > > > > > > > > On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols > wrote: > > > > > To package Geode as Java 11 Jigsaw module(s), a major hurdle is the > > > requirement that packages cannot be split across modules. > > > > > > If we simply map existing Geode jars to Modules, we have about 10 split > > > packages (see table below). Any restructuring would potentially have > to > > > wait until Geode 2.0. > > > > > > A workaround would be to map existing packages into a small number of > new > > > modules (e.g. geode-api and geode-internal). Existing jars would > remain > > > as-is. Users making the transition to Java 11 /w Jigsaw already need > to > > > switch from CLASSPATH to MODULEPATH so the inconvenience of a > different > > > naming scheme for Geode-as-modules might be acceptable (however, once > > > module naming and organization is established, we may be stuck with it > > for > > > a long time). > > > > > > Thoughts? > > > > > > Is it even possible to rearrange all classes in each package below > into a > > > single jar? Is doing so desirable enough to defer Java 11 support > until > > a > > > yet-unplanned Geode 2.0, or perhaps to drive such a release? > > > > > > Or, is it satisfactory to fatten Geode releases to include one set of > > jars > > > for CLASSPATH use, plus a different set of jars for MODULEPATH use? > > > > > > > > > Package > > > Jar > > > org.apache.geode.cache.client.internal > > > geode-core-1.8.0.jar > > > geode-cq-1.8.0.jar > > > geode-wan-1.8.0.jar > > > org.apache.geode.cache.client.internal.locator.wan > > > geode-core-1.8.0.jar > > > geode-wan-1.8.0.jar > > > org.apache.geode.cache.query.internal.cq > > > geode-core-1.8.0.jar > > > geode-cq-1.8.0.jar > > >
Re: Adding index for Like operartor
Key indexes tell the query engine that the field is the same as the key that was used to put into the underlying region. So let's say your field is "ID". Creating a key index tells the query engine that a "select * from /region theRegion where ID = 1" is the same as doing : theRegion.get(1). In this case I think you are looking for a functional index. Those are sorted and will be used for like queries. On Mon, Nov 26, 2018 at 12:52 PM Alexander Murmann wrote: > Hi, > > Have you thought about using the built-in Lucene functionality for this? > Fuzzy text search is exactly what it's made for. > > On Mon, Nov 26, 2018 at 12:15 PM anjana_nair > wrote: > > > > > Hi, > > > > I would like to add an index to key (keyindex) and the query executed is > > going to be like > > > > "constantstring%" where constant string is going to be the constant > > prefix in a lile query. In this case can we just go ahead with key index > > or > > should we use a function index ? key indexes are not sorted ? > > > > > > > > -- > > Sent from: > > http://apache-geode-incubating-developers-forum.70738.x6.nabble.com/ > > >
Re: Adding index for Like operartor
Hi, Have you thought about using the built-in Lucene functionality for this? Fuzzy text search is exactly what it's made for. On Mon, Nov 26, 2018 at 12:15 PM anjana_nair wrote: > > Hi, > > I would like to add an index to key (keyindex) and the query executed is > going to be like > > "constantstring%" where constant string is going to be the constant > prefix in a lile query. In this case can we just go ahead with key index > or > should we use a function index ? key indexes are not sorted ? > > > > -- > Sent from: > http://apache-geode-incubating-developers-forum.70738.x6.nabble.com/ >
Re: AcceptanceTestOpenJDK11 precheckin fails to compile
I believe that the unable to access file: corrupted zip file indicates some transient failure in GCP? I remember seeing it in the past, but I don't think we ever got to the bottom of it. To respond to your other email, yeah, I think bumping your PR to re-fire might be the best way to proceed? As it stands, since the build failed, none of the JDK11 tests would've run, which mildly worries me, even if the build failure is understood. We could look into separating the build and test tasks in Concourse, so that the build step would retry on these failures. I'm not sure if it would end up caching things in a way that would cause us to not actually ameliorate the issue, though. tl;dr: I think you just rolled snake-eyes. Imagination is Change. ~Patrick On Mon, Nov 26, 2018 at 12:20 PM Kirk Lund wrote: > Just the first few compilation failures are listed below. Is the image > for AcceptanceTestOpenJDK11 precheckin broken in some way? > > > /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/InternalInstantiator.java:20: > error: cannot access Constructor > import java.lang.reflect.Constructor; > ^ > bad class file: > > /usr/lib/jvm/java-8-openjdk-amd64/lib/ct.sym(META-INF/sym/rt.jar/java/lang/reflect/Constructor.class) > unable to access file: corrupted zip file > Please remove or make sure it appears in the correct subdirectory of > the classpath. > > /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java:18: > error: cannot find symbol > import static > > org.apache.geode.internal.offheap.annotations.OffHeapIdentifier.ENTRY_EVENT_NEW_VALUE; > ^ > symbol: static ENTRY_EVENT_NEW_VALUE > location: class > > /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java:17: > error: cannot find symbol > import static org.apache.geode.internal.lang.SystemUtils.getLineSeparator; > ^ > symbol: static getLineSeparator > location: class >
Re: AcceptanceTestOpenJDK11 precheckin fails to compile
That’s really odd because that jar is baked into the build image. If it was truly corrupt then all builds should be failing. Hmmm > On Nov 26, 2018, at 12:19 PM, Kirk Lund wrote: > > Just the first few compilation failures are listed below. Is the image > for AcceptanceTestOpenJDK11 precheckin broken in some way? > > /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/InternalInstantiator.java:20: > error: cannot access Constructor > import java.lang.reflect.Constructor; >^ > bad class file: > /usr/lib/jvm/java-8-openjdk-amd64/lib/ct.sym(META-INF/sym/rt.jar/java/lang/reflect/Constructor.class) >unable to access file: corrupted zip file >Please remove or make sure it appears in the correct subdirectory of > the classpath. > /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java:18: > error: cannot find symbol > import static > org.apache.geode.internal.offheap.annotations.OffHeapIdentifier.ENTRY_EVENT_NEW_VALUE; > ^ > symbol: static ENTRY_EVENT_NEW_VALUE > location: class > /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java:17: > error: cannot find symbol > import static org.apache.geode.internal.lang.SystemUtils.getLineSeparator; > ^ > symbol: static getLineSeparator > location: class
Unit test target loads up full Geode logging
The unit test target has geode-core/src/main/resources in the classpath which means that Log4J finds our log4j2.xml file and creates/registers our custom appenders. We might want to have a unit test log4j2.xml that gets placed earlier on the classpath but only for the unit test target. The integrationTest and distributedTest targets still need the geode-core/src/main/resources/log4j2.xml earliest on the classpath. The best solution is probably to hide geode-core/src/main/resources/log4j2.xml from unit test if that's possible. Second best solution might be to have a resources src set containing a simplified log4j2.xml which is added to the classpath for unit test but not for the other testing targets. Anyone with gradle knowledge interested in helping with this?
Re: New Geode PMC Chair: Karen Miller
Thanks, Mark - Welcome Karen! On Mon, Nov 26, 2018 at 12:17 PM Dan Smith wrote: > Congratulations Karen! And thanks for all of your hard work, Mark! > > -Dan > > On Mon, Nov 26, 2018 at 11:56 AM Mark Bretl wrote: > > > Hello Geode Community, > > > > After two years, the Project Management Committee (PMC) agreed to > > transition the role of PMC chair from me to another PMC member. I would > > like to announce, at last week's Apache Board Meeting, the Board approved > > the Geode PMC's recommendation of Karen Miller to the office of V.P., > > Apache Geode. > > > > Please welcome our new PMC Chair, Karen Miller! > > > > Best regards, > > > > --Mark > > >
AcceptanceTestOpenJDK11 precheckin fails to compile
Just the first few compilation failures are listed below. Is the image for AcceptanceTestOpenJDK11 precheckin broken in some way? /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/InternalInstantiator.java:20: error: cannot access Constructor import java.lang.reflect.Constructor; ^ bad class file: /usr/lib/jvm/java-8-openjdk-amd64/lib/ct.sym(META-INF/sym/rt.jar/java/lang/reflect/Constructor.class) unable to access file: corrupted zip file Please remove or make sure it appears in the correct subdirectory of the classpath. /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java:18: error: cannot find symbol import static org.apache.geode.internal.offheap.annotations.OffHeapIdentifier.ENTRY_EVENT_NEW_VALUE; ^ symbol: static ENTRY_EVENT_NEW_VALUE location: class /home/geode/geode/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java:17: error: cannot find symbol import static org.apache.geode.internal.lang.SystemUtils.getLineSeparator; ^ symbol: static getLineSeparator location: class
Re: New Geode PMC Chair: Karen Miller
Congratulations Karen! And thanks for all of your hard work, Mark! -Dan On Mon, Nov 26, 2018 at 11:56 AM Mark Bretl wrote: > Hello Geode Community, > > After two years, the Project Management Committee (PMC) agreed to > transition the role of PMC chair from me to another PMC member. I would > like to announce, at last week's Apache Board Meeting, the Board approved > the Geode PMC's recommendation of Karen Miller to the office of V.P., > Apache Geode. > > Please welcome our new PMC Chair, Karen Miller! > > Best regards, > > --Mark >
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
*> Most of these are in internal packages, which means we can change the package of these classes without breaking users.* I don't agree with this. Some functionality in Geode required by an application, or framework, is only available in an "internal" package, unfortunately. This has to do with the fact that many interfaces in Geode were not well designed. In general, and as a general rule of thumb, anytime you change a method name or method signature, it is an API breaking change. Adding a method to an interface, is an API breaking change. Removing methods from an interface is (potentially) an API breaking change. Changing a type is an API breaking change. There are several other examples. Any class/method (dare I say field of a class) that is not "properly protected" is part of your API, regardless if we deem it be "internal" only or not, and (possibly) will affect users. I think any level of modularity (and maturing) is pointless without 1) first, having well defined contracts/interfaces between functional areas (based on concerns) and 2) using appropriate access modifiers and cleanly delineating APIs/SPIs from implementations/providers. -j On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: > I think we can actually fix most of these issues without geode-2.0. Most of > these are in internal packages, which means we can change the package of > these classes without breaking users. The only concerning one is > org.apache.geode.cache.util, which is a public package. However, the > AutoBalancer is actually still marked @Experimental, so we could > technically move that as well. Or maybe deprecate the old one and it's > associated jar, and create a new jar with a reasonable package. JDK11 users > could just avoid the old jar. > > I have been wondering for a while if we should just fold geode-cq and > geode-wan back into geode-core. These are not really properly separated > out, they are very tangled with core. The above package overlap kinda shows > that as well. But maybe going through the effort of renaming the packages > to make them java 11 modules would help improve the code anyway! > > -Dan > > > > > On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols wrote: > > > To package Geode as Java 11 Jigsaw module(s), a major hurdle is the > > requirement that packages cannot be split across modules. > > > > If we simply map existing Geode jars to Modules, we have about 10 split > > packages (see table below). Any restructuring would potentially have to > > wait until Geode 2.0. > > > > A workaround would be to map existing packages into a small number of new > > modules (e.g. geode-api and geode-internal). Existing jars would remain > > as-is. Users making the transition to Java 11 /w Jigsaw already need to > > switch from CLASSPATH to MODULEPATH so the inconvenience of a different > > naming scheme for Geode-as-modules might be acceptable (however, once > > module naming and organization is established, we may be stuck with it > for > > a long time). > > > > Thoughts? > > > > Is it even possible to rearrange all classes in each package below into a > > single jar? Is doing so desirable enough to defer Java 11 support until > a > > yet-unplanned Geode 2.0, or perhaps to drive such a release? > > > > Or, is it satisfactory to fatten Geode releases to include one set of > jars > > for CLASSPATH use, plus a different set of jars for MODULEPATH use? > > > > > > Package > > Jar > > org.apache.geode.cache.client.internal > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.client.internal.locator.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.query.internal.cq > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.cache.util > > geode-core-1.8.0.jar > > geode-rebalancer-1.8.0.jar > > org.apache.geode.internal > > geode-connectors-1.8.0.jar > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-lucene-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.tier.sockets.command > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.internal.cache.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.parallel > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.serial > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.protocol.protobuf.v1 > > geode-protobuf-1.8.0.jar > > geode-protobuf-messages-1.8.0.jar > -- -John john.blum10101 (skype)
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
I need more time to think about this clearly (currently juggling multiple things), but I would say this... I don't think there should be "internal" APIs, as in o.a.g.*.internal. There are only APIs/SPIs and implementations, period. If you want to maintain an API as internal, then you should properly protect that API with the appropriate *Java* access modifiers (private, package-private and protected). -j On Mon, Nov 26, 2018 at 11:29 AM Jacob Barrett wrote: > On Mon, Nov 26, 2018 at 11:23 AM Udo Kohlmeyer > wrote: > > > Imo, no module should export "internal". Maybe a stretch goal, but > > something we should keep in mind. > > > > Yes, agreed, but outside the scope of this current discussion. > -- -John john.blum10101 (skype)
Re: Avoiding Unpredictability in Our DUnit Testing Rules
Typo fix: If you need to use CleanupDUnitVMsRule along with other dunit rules, then you will need to* use RuleChain*. If you don't need to use CleanupDUnitVMsRule then you don't need to use RuleChain. On Mon, Nov 26, 2018 at 11:57 AM Kirk Lund wrote: > Actually the only problem with this is specific to bouncing of dunit VMs. > I filed "GEODE-6033: DistributedTest rules should support VM bounce" > earlier this month and I have a branch with preliminary changes that seem > to be working fine. > > Aside from bouncing of dunit VMs, the dunit rules you listed do not care > about ordering of invocation, so you do *not* need to use RuleChain > (unless you're using CleanupDUnitVMsRule). There are two caveats though: > > 1) if you want or need to specify the number of VMs to be something other > than the default of 4, then you'll need to specify the number of VMs for > every one of these dunit rules (see example below) > > 2) these dunit rules do not currently work with bouncing of vms > > If you need to use CleanupDUnitVMsRule along with other dunit rules, then > you will need to use. If you need to use VM bouncing within a test, then > you'll need to wait until I can merge my > (I have a branch on which I've made some changes to make this work) > > So it's actually #2 that's causing your problem. But as I mentioned, I do > have a branch on which it does now work with bouncing of VMs but I'm not > quite ready to merge it. > > Here's an example for a test that wants to limit the number of dunit VMs > to 2. RuleChain is not needed, but you do have to specify the # of dunit > VMs on all 3 dunit rules: > > @Rule > public DistributedRule distributedRule = new DistributedRule(2); > > @Rule > public SharedCountersRule sharedCountersRule = new SharedCountersRule(2); > > @Rule > public SharedErrorCollector errorCollector = new SharedErrorCollector(2); > > Please don't use ManagementTestRule at all. I'm trying to modify all tests > that use it to use DistributedRule with Geode User APIs so that I can > delete it. > > All of those other dunit rules in your list extend AbstractDistributedRule > which is why rule ordering does not matter (unless you use > CleanupDUnitVMsRule because it bounces VMs). > > Thanks, > Kirk > > On Wed, Nov 21, 2018 at 10:34 AM Patrick Rhomberg > wrote: > >> tl;dr: Use JUnit RuleChain. >> >> >> Hello all! >> >> Several [1] of our test @Rule classes make use of the fact that our DUnit >> VMs Host is statically accessible to affect every test VM. For instance, >> the SharedCountersRule initializes a counter in every VM, and the >> CleanupDUnitVMsRule bounces VMs before and after each test. >> >> Problematically, JUnit rules applied in an unpredictable / JVM-dependent >> ordering. [2] As a result, some flakiness we find in our tests may be the >> result of unexpected interaction and ordering of our test rules. [3] >> >> Fortunately, a solution to this problem already exists. Rule ordering can >> be imposed by JUnit's RuleChain. [4] >> >> In early exploration with this rule, some tests failed due to the >> RuleChain >> not being serializable. However, as it should only apply to the test VM, >> and given that it can be composed of (unannotated) rules that remain >> accessible and serializable, it should be a simple matter of declaring the >> offending field transient, as it will only be necessary in the test VM. >> >> So, you dear reader: while you're out there making Geode the best it can >> be, if you find yourself in a test class that uses more than one rule >> listed in [1], or if you notice some other rule not listed below that >> reaches out to VMs as part of its @Before or @After, please update that >> test to use the RuleChain to apply the rules in a predictable order. >> >> Imagination is Change. >> ~Patrick >> >> [1] A probably-incomplete list of invasive rules can be found via >> $> git grep -il inEveryVM | grep Rule.java >> >> geode-core/src/distributedTest/java/org/apache/geode/management/ManagementTestRule.java >> geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/CacheRule.java >> >> geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/ClientCacheRule.java >> >> geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedDiskDirRule.java >> >> geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedRule.java >> >> geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedUseJacksonForJsonPathRule.java >> >> geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/SharedCountersRule.java >> >> [2] See the documentation for rules here: >> https://junit.org/junit4/javadoc/4.12/org/junit/Rule.html ; notably, >> "However, >> if there are multiple [Rule] fields (or methods) they will be applied in >> an >> order that depends on your JVM's implementation of the reflection API, >> which is undefined, in general." >> >> [3] For what it's worth, this was discovered after looking into why the >> DistributedRule check
Re: Avoiding Unpredictability in Our DUnit Testing Rules
Actually the only problem with this is specific to bouncing of dunit VMs. I filed "GEODE-6033: DistributedTest rules should support VM bounce" earlier this month and I have a branch with preliminary changes that seem to be working fine. Aside from bouncing of dunit VMs, the dunit rules you listed do not care about ordering of invocation, so you do *not* need to use RuleChain (unless you're using CleanupDUnitVMsRule). There are two caveats though: 1) if you want or need to specify the number of VMs to be something other than the default of 4, then you'll need to specify the number of VMs for every one of these dunit rules (see example below) 2) these dunit rules do not currently work with bouncing of vms If you need to use CleanupDUnitVMsRule along with other dunit rules, then you will need to use. If you need to use VM bouncing within a test, then you'll need to wait until I can merge my (I have a branch on which I've made some changes to make this work) So it's actually #2 that's causing your problem. But as I mentioned, I do have a branch on which it does now work with bouncing of VMs but I'm not quite ready to merge it. Here's an example for a test that wants to limit the number of dunit VMs to 2. RuleChain is not needed, but you do have to specify the # of dunit VMs on all 3 dunit rules: @Rule public DistributedRule distributedRule = new DistributedRule(2); @Rule public SharedCountersRule sharedCountersRule = new SharedCountersRule(2); @Rule public SharedErrorCollector errorCollector = new SharedErrorCollector(2); Please don't use ManagementTestRule at all. I'm trying to modify all tests that use it to use DistributedRule with Geode User APIs so that I can delete it. All of those other dunit rules in your list extend AbstractDistributedRule which is why rule ordering does not matter (unless you use CleanupDUnitVMsRule because it bounces VMs). Thanks, Kirk On Wed, Nov 21, 2018 at 10:34 AM Patrick Rhomberg wrote: > tl;dr: Use JUnit RuleChain. > > > Hello all! > > Several [1] of our test @Rule classes make use of the fact that our DUnit > VMs Host is statically accessible to affect every test VM. For instance, > the SharedCountersRule initializes a counter in every VM, and the > CleanupDUnitVMsRule bounces VMs before and after each test. > > Problematically, JUnit rules applied in an unpredictable / JVM-dependent > ordering. [2] As a result, some flakiness we find in our tests may be the > result of unexpected interaction and ordering of our test rules. [3] > > Fortunately, a solution to this problem already exists. Rule ordering can > be imposed by JUnit's RuleChain. [4] > > In early exploration with this rule, some tests failed due to the RuleChain > not being serializable. However, as it should only apply to the test VM, > and given that it can be composed of (unannotated) rules that remain > accessible and serializable, it should be a simple matter of declaring the > offending field transient, as it will only be necessary in the test VM. > > So, you dear reader: while you're out there making Geode the best it can > be, if you find yourself in a test class that uses more than one rule > listed in [1], or if you notice some other rule not listed below that > reaches out to VMs as part of its @Before or @After, please update that > test to use the RuleChain to apply the rules in a predictable order. > > Imagination is Change. > ~Patrick > > [1] A probably-incomplete list of invasive rules can be found via > $> git grep -il inEveryVM | grep Rule.java > > geode-core/src/distributedTest/java/org/apache/geode/management/ManagementTestRule.java > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/CacheRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/ClientCacheRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedDiskDirRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedUseJacksonForJsonPathRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/SharedCountersRule.java > > [2] See the documentation for rules here: > https://junit.org/junit4/javadoc/4.12/org/junit/Rule.html ; notably, > "However, > if there are multiple [Rule] fields (or methods) they will be applied in an > order that depends on your JVM's implementation of the reflection API, > which is undefined, in general." > > [3] For what it's worth, this was discovered after looking into why the > DistributedRule check fo suspicious strings caused the test *after* the > test that emitted the strings to fail. It's only tangentially related, but > got me looking into when and how the @After was getting applied. > > [4] https://junit.org/junit4/javadoc/4.12/org/junit/rules/RuleChain.html >
New Geode PMC Chair: Karen Miller
Hello Geode Community, After two years, the Project Management Committee (PMC) agreed to transition the role of PMC chair from me to another PMC member. I would like to announce, at last week's Apache Board Meeting, the Board approved the Geode PMC's recommendation of Karen Miller to the office of V.P., Apache Geode. Please welcome our new PMC Chair, Karen Miller! Best regards, --Mark
Adding index for Like operartor
Hi, I would like to add an index to key (keyindex) and the query executed is going to be like "constantstring%" where constant string is going to be the constant prefix in a lile query. In this case can we just go ahead with key index or should we use a function index ? key indexes are not sorted ? -- Sent from: http://apache-geode-incubating-developers-forum.70738.x6.nabble.com/
Re: Avoiding Unpredictability in Our DUnit Testing Rules
Thanks for the correction Bruce! > On Nov 26, 2018, at 10:04 AM, Bruce Schuchardt wrote: > > While it's common to use VM.getVM(int) to get a VM you still must use > Host.getHost() to get a VM running an older version of Geode. Until someone > adds a VM.getVM(version, vmNumber) we can't frown upon > Host.getHost(0).getVM(startingVersion, 0). > > On 11/21/18 4:09 PM, Mark Hanson wrote: >> It is frowned upon. VM.getVM(0) is now the accepted way to get VM 0. >> >> Thanks, >> Mark >> >> >>> On Nov 21, 2018, at 10:41 AM, Nabarun Nag wrote: >>> >>> Will doing this in the test, >>> >>> final Host host = Host.getHost(0); >>> VM server1 = host.getVM(startingVersion, 0); >>> >>> be frowned upon, if I use the above over using @Rule. >>> >>> Regards >>> Nabarun Nag >>> On Nov 21, 2018, at 10:36 AM, Robert Houghton wrote: Great find, Patrick. I hope this shakes out some of the test bugs! On Wed, Nov 21, 2018, 10:34 Patrick Rhomberg >>> > tl;dr: Use JUnit RuleChain. > > > Hello all! > > Several [1] of our test @Rule classes make use of the fact that our DUnit > VMs Host is statically accessible to affect every test VM. For instance, > the SharedCountersRule initializes a counter in every VM, and the > CleanupDUnitVMsRule bounces VMs before and after each test. > > Problematically, JUnit rules applied in an unpredictable / JVM-dependent > ordering. [2] As a result, some flakiness we find in our tests may be the > result of unexpected interaction and ordering of our test rules. [3] > > Fortunately, a solution to this problem already exists. Rule ordering can > be imposed by JUnit's RuleChain. [4] > > In early exploration with this rule, some tests failed due to the > RuleChain > not being serializable. However, as it should only apply to the test VM, > and given that it can be composed of (unannotated) rules that remain > accessible and serializable, it should be a simple matter of declaring the > offending field transient, as it will only be necessary in the test VM. > > So, you dear reader: while you're out there making Geode the best it can > be, if you find yourself in a test class that uses more than one rule > listed in [1], or if you notice some other rule not listed below that > reaches out to VMs as part of its @Before or @After, please update that > test to use the RuleChain to apply the rules in a predictable order. > > Imagination is Change. > ~Patrick > > [1] A probably-incomplete list of invasive rules can be found via > $> git grep -il inEveryVM | grep Rule.java > > geode-core/src/distributedTest/java/org/apache/geode/management/ManagementTestRule.java > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/CacheRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/ClientCacheRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedDiskDirRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedUseJacksonForJsonPathRule.java > > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/SharedCountersRule.java > > [2] See the documentation for rules here: > https://junit.org/junit4/javadoc/4.12/org/junit/Rule.html ; notably, > "However, > if there are multiple [Rule] fields (or methods) they will be applied in > an > order that depends on your JVM's implementation of the reflection API, > which is undefined, in general." > > [3] For what it's worth, this was discovered after looking into why the > DistributedRule check fo suspicious strings caused the test *after* the > test that emitted the strings to fail. It's only tangentially related, > but > got me looking into when and how the @After was getting applied. > > [4] https://junit.org/junit4/javadoc/4.12/org/junit/rules/RuleChain.html >
Re: JIRA write access?
Done! -Dan On Mon, Nov 26, 2018 at 11:15 AM Sean Goller wrote: > Hi, In order to start dealing with some JIRA tickets could I get assign and > resolve access? > My apache username is smgoller. > > Thanks! > > -Sean. >
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
On Mon, Nov 26, 2018 at 11:23 AM Udo Kohlmeyer wrote: > Imo, no module should export "internal". Maybe a stretch goal, but > something we should keep in mind. > Yes, agreed, but outside the scope of this current discussion.
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
Imo, no module should export "internal". Maybe a stretch goal, but something we should keep in mind. On 11/26/18 10:50, Jacob Barrett wrote: One lingering question I have around jigsaw is the split package issue recursive? In that I mean if a module exports "org.apache.geode.internal" and another module exports "org.apache.geode.internal.foo" is this legal? On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: I think we can actually fix most of these issues without geode-2.0. Most of these are in internal packages, which means we can change the package of these classes without breaking users. The only concerning one is org.apache.geode.cache.util, which is a public package. However, the AutoBalancer is actually still marked @Experimental, so we could technically move that as well. Or maybe deprecate the old one and it's associated jar, and create a new jar with a reasonable package. JDK11 users could just avoid the old jar. I have been wondering for a while if we should just fold geode-cq and geode-wan back into geode-core. These are not really properly separated out, they are very tangled with core. The above package overlap kinda shows that as well. But maybe going through the effort of renaming the packages to make them java 11 modules would help improve the code anyway! -Dan On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols wrote: To package Geode as Java 11 Jigsaw module(s), a major hurdle is the requirement that packages cannot be split across modules. If we simply map existing Geode jars to Modules, we have about 10 split packages (see table below). Any restructuring would potentially have to wait until Geode 2.0. A workaround would be to map existing packages into a small number of new modules (e.g. geode-api and geode-internal). Existing jars would remain as-is. Users making the transition to Java 11 /w Jigsaw already need to switch from CLASSPATH to MODULEPATH so the inconvenience of a different naming scheme for Geode-as-modules might be acceptable (however, once module naming and organization is established, we may be stuck with it for a long time). Thoughts? Is it even possible to rearrange all classes in each package below into a single jar? Is doing so desirable enough to defer Java 11 support until a yet-unplanned Geode 2.0, or perhaps to drive such a release? Or, is it satisfactory to fatten Geode releases to include one set of jars for CLASSPATH use, plus a different set of jars for MODULEPATH use? Package Jar org.apache.geode.cache.client.internal geode-core-1.8.0.jar geode-cq-1.8.0.jar geode-wan-1.8.0.jar org.apache.geode.cache.client.internal.locator.wan geode-core-1.8.0.jar geode-wan-1.8.0.jar org.apache.geode.cache.query.internal.cq geode-core-1.8.0.jar geode-cq-1.8.0.jar org.apache.geode.cache.util geode-core-1.8.0.jar geode-rebalancer-1.8.0.jar org.apache.geode.internal geode-connectors-1.8.0.jar geode-core-1.8.0.jar geode-cq-1.8.0.jar geode-lucene-1.8.0.jar geode-wan-1.8.0.jar org.apache.geode.internal.cache.tier.sockets.command geode-core-1.8.0.jar geode-cq-1.8.0.jar org.apache.geode.internal.cache.wan geode-core-1.8.0.jar geode-wan-1.8.0.jar org.apache.geode.internal.cache.wan.parallel geode-core-1.8.0.jar geode-wan-1.8.0.jar org.apache.geode.internal.cache.wan.serial geode-core-1.8.0.jar geode-wan-1.8.0.jar org.apache.geode.internal.protocol.protobuf.v1 geode-protobuf-1.8.0.jar geode-protobuf-messages-1.8.0.jar
JIRA write access?
Hi, In order to start dealing with some JIRA tickets could I get assign and resolve access? My apache username is smgoller. Thanks! -Sean.
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
If Dan's assertions are correct, and we can in fact refactor all these packages without API changes, then I am all for giving it a shot. Worst case we have done a little of the work we will have to do for a 2.0 api change anyway and have to revert to the idea of an uber jar. On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: > I think we can actually fix most of these issues without geode-2.0. Most of > these are in internal packages, which means we can change the package of > these classes without breaking users. The only concerning one is > org.apache.geode.cache.util, which is a public package. However, the > AutoBalancer is actually still marked @Experimental, so we could > technically move that as well. Or maybe deprecate the old one and it's > associated jar, and create a new jar with a reasonable package. JDK11 users > could just avoid the old jar. > > I have been wondering for a while if we should just fold geode-cq and > geode-wan back into geode-core. These are not really properly separated > out, they are very tangled with core. The above package overlap kinda shows > that as well. But maybe going through the effort of renaming the packages > to make them java 11 modules would help improve the code anyway! > > -Dan > > > > > On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols wrote: > > > To package Geode as Java 11 Jigsaw module(s), a major hurdle is the > > requirement that packages cannot be split across modules. > > > > If we simply map existing Geode jars to Modules, we have about 10 split > > packages (see table below). Any restructuring would potentially have to > > wait until Geode 2.0. > > > > A workaround would be to map existing packages into a small number of new > > modules (e.g. geode-api and geode-internal). Existing jars would remain > > as-is. Users making the transition to Java 11 /w Jigsaw already need to > > switch from CLASSPATH to MODULEPATH so the inconvenience of a different > > naming scheme for Geode-as-modules might be acceptable (however, once > > module naming and organization is established, we may be stuck with it > for > > a long time). > > > > Thoughts? > > > > Is it even possible to rearrange all classes in each package below into a > > single jar? Is doing so desirable enough to defer Java 11 support until > a > > yet-unplanned Geode 2.0, or perhaps to drive such a release? > > > > Or, is it satisfactory to fatten Geode releases to include one set of > jars > > for CLASSPATH use, plus a different set of jars for MODULEPATH use? > > > > > > Package > > Jar > > org.apache.geode.cache.client.internal > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.client.internal.locator.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.query.internal.cq > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.cache.util > > geode-core-1.8.0.jar > > geode-rebalancer-1.8.0.jar > > org.apache.geode.internal > > geode-connectors-1.8.0.jar > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-lucene-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.tier.sockets.command > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.internal.cache.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.parallel > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.serial > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.protocol.protobuf.v1 > > geode-protobuf-1.8.0.jar > > geode-protobuf-messages-1.8.0.jar >
Re: [DISCUSS] Geode packages mustn't span Jigsaw modules
One lingering question I have around jigsaw is the split package issue recursive? In that I mean if a module exports "org.apache.geode.internal" and another module exports "org.apache.geode.internal.foo" is this legal? On Mon, Nov 19, 2018 at 5:21 PM Dan Smith wrote: > I think we can actually fix most of these issues without geode-2.0. Most of > these are in internal packages, which means we can change the package of > these classes without breaking users. The only concerning one is > org.apache.geode.cache.util, which is a public package. However, the > AutoBalancer is actually still marked @Experimental, so we could > technically move that as well. Or maybe deprecate the old one and it's > associated jar, and create a new jar with a reasonable package. JDK11 users > could just avoid the old jar. > > I have been wondering for a while if we should just fold geode-cq and > geode-wan back into geode-core. These are not really properly separated > out, they are very tangled with core. The above package overlap kinda shows > that as well. But maybe going through the effort of renaming the packages > to make them java 11 modules would help improve the code anyway! > > -Dan > > > > > On Mon, Nov 19, 2018 at 5:03 PM Owen Nichols wrote: > > > To package Geode as Java 11 Jigsaw module(s), a major hurdle is the > > requirement that packages cannot be split across modules. > > > > If we simply map existing Geode jars to Modules, we have about 10 split > > packages (see table below). Any restructuring would potentially have to > > wait until Geode 2.0. > > > > A workaround would be to map existing packages into a small number of new > > modules (e.g. geode-api and geode-internal). Existing jars would remain > > as-is. Users making the transition to Java 11 /w Jigsaw already need to > > switch from CLASSPATH to MODULEPATH so the inconvenience of a different > > naming scheme for Geode-as-modules might be acceptable (however, once > > module naming and organization is established, we may be stuck with it > for > > a long time). > > > > Thoughts? > > > > Is it even possible to rearrange all classes in each package below into a > > single jar? Is doing so desirable enough to defer Java 11 support until > a > > yet-unplanned Geode 2.0, or perhaps to drive such a release? > > > > Or, is it satisfactory to fatten Geode releases to include one set of > jars > > for CLASSPATH use, plus a different set of jars for MODULEPATH use? > > > > > > Package > > Jar > > org.apache.geode.cache.client.internal > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.client.internal.locator.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.cache.query.internal.cq > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.cache.util > > geode-core-1.8.0.jar > > geode-rebalancer-1.8.0.jar > > org.apache.geode.internal > > geode-connectors-1.8.0.jar > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > geode-lucene-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.tier.sockets.command > > geode-core-1.8.0.jar > > geode-cq-1.8.0.jar > > org.apache.geode.internal.cache.wan > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.parallel > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.cache.wan.serial > > geode-core-1.8.0.jar > > geode-wan-1.8.0.jar > > org.apache.geode.internal.protocol.protobuf.v1 > > geode-protobuf-1.8.0.jar > > geode-protobuf-messages-1.8.0.jar >
Re: What is the new process for flaky test failures in precheckin?
For test failures that do not have a ticket for that particular stack trace, you should re-trigger your pre-checkin. If the test fails again, your change probably caused it to start failing, even if it doesn't seem related (like the unit test ordering issue we had Friday before last), and you would be expected to fix it before committing. If the test passes your next run of tests the process is less established. Ideally we would never encounter this case because it means we have checked in flaky code at some point before your branch off of develop, which StressNewTest should catch. We will probably find a few of these because the StressNewTest job was silently failing for a little while. It is working again, but we may have checked in some flaky tests during the time it was down. I propose that we follow the CIO process with these. The process would be as follows: 1. Create a Jira ticket for the failure with links to the relevant resources from the failing CI run. Include any evidence that you have towards it being a flaky test that exists on develop and not just in your branch, and evidence that it was not made flaky by your change. 2. See if that test file was changed recently. If it was, talk to the person that changed it. 3. If it wasn't changed recently, post a link to gemfire-green-ci 4. Comment on your pull request with an update on the status of the failing test in your run. That is just an idea based on what we are doing for the develop pipeline. It seems like we are having more failures in PR pipelines than we were seeing a couple of weeks ago, and some tests that seem to only fail in the PR pipelines, so it might be time to start tracking some of this stuff. My main concern with this method is that it might pollute our backlog with tickets that no one is ever going to look at. On Mon, Nov 26, 2018 at 10:19 AM Kirk Lund wrote: > I just saw SizingFlagDUnitTest fail in a precheckin but it passes on my > branch when I run directly. I cannot find a Jira ticket for it. What's the > new process for handling these flickering tests? > > See: > https://concourse.apachegeode-ci.info/builds/17745 > > Test failure stack: > org.apache.geode.internal.cache.SizingFlagDUnitTest > > testPRHeapLRUDeltaPutOnPrimary FAILED > org.apache.geode.test.dunit.RMIException: While invoking > org.apache.geode.internal.cache.SizingFlagDUnitTest$12.run in VM 0 running > on Host eb7aca4f2587 with 4 VMs > at org.apache.geode.test.dunit.VM.invoke(VM.java:433) > at org.apache.geode.test.dunit.VM.invoke(VM.java:402) > at org.apache.geode.test.dunit.VM.invoke(VM.java:361) > at > > org.apache.geode.internal.cache.SizingFlagDUnitTest.assertValueType(SizingFlagDUnitTest.java:793) > at > > org.apache.geode.internal.cache.SizingFlagDUnitTest.doPRDeltaTestLRU(SizingFlagDUnitTest.java:312) > at > > org.apache.geode.internal.cache.SizingFlagDUnitTest.testPRHeapLRUDeltaPutOnPrimary(SizingFlagDUnitTest.java:220) > > Caused by: > org.apache.geode.cache.EntryNotFoundException: Entry not found for > key 0 > at > > org.apache.geode.internal.cache.LocalRegion.checkEntryNotFound(LocalRegion.java:2760) > at > > org.apache.geode.internal.cache.LocalRegion.nonTXbasicGetValueInVM(LocalRegion.java:3448) > at > > org.apache.geode.internal.cache.LocalRegionDataView.getValueInVM(LocalRegionDataView.java:105) > at > > org.apache.geode.internal.cache.LocalRegion.basicGetValueInVM(LocalRegion.java:3436) > at > > org.apache.geode.internal.cache.LocalRegion.getValueInVM(LocalRegion.java:3424) > at > > org.apache.geode.internal.cache.PartitionedRegionDataStore.getLocalValueInVM(PartitionedRegionDataStore.java:2775) > at > > org.apache.geode.internal.cache.PartitionedRegion.getValueInVM(PartitionedRegion.java:8786) > at > > org.apache.geode.internal.cache.SizingFlagDUnitTest$12.run(SizingFlagDUnitTest.java:797) >
Handling precheckin jobs with concourse errors
The DistributedTestOpenJDK11 precheckin job for PR #2878 failed with what I think is a Concourse error: https://github.com/apache/geode/pull/2878 I believe the expected process from me is to just poke my PR to repeat precheckin. If that's wrong, please let me know. Thanks!
What is the new process for flaky test failures in precheckin?
I just saw SizingFlagDUnitTest fail in a precheckin but it passes on my branch when I run directly. I cannot find a Jira ticket for it. What's the new process for handling these flickering tests? See: https://concourse.apachegeode-ci.info/builds/17745 Test failure stack: org.apache.geode.internal.cache.SizingFlagDUnitTest > testPRHeapLRUDeltaPutOnPrimary FAILED org.apache.geode.test.dunit.RMIException: While invoking org.apache.geode.internal.cache.SizingFlagDUnitTest$12.run in VM 0 running on Host eb7aca4f2587 with 4 VMs at org.apache.geode.test.dunit.VM.invoke(VM.java:433) at org.apache.geode.test.dunit.VM.invoke(VM.java:402) at org.apache.geode.test.dunit.VM.invoke(VM.java:361) at org.apache.geode.internal.cache.SizingFlagDUnitTest.assertValueType(SizingFlagDUnitTest.java:793) at org.apache.geode.internal.cache.SizingFlagDUnitTest.doPRDeltaTestLRU(SizingFlagDUnitTest.java:312) at org.apache.geode.internal.cache.SizingFlagDUnitTest.testPRHeapLRUDeltaPutOnPrimary(SizingFlagDUnitTest.java:220) Caused by: org.apache.geode.cache.EntryNotFoundException: Entry not found for key 0 at org.apache.geode.internal.cache.LocalRegion.checkEntryNotFound(LocalRegion.java:2760) at org.apache.geode.internal.cache.LocalRegion.nonTXbasicGetValueInVM(LocalRegion.java:3448) at org.apache.geode.internal.cache.LocalRegionDataView.getValueInVM(LocalRegionDataView.java:105) at org.apache.geode.internal.cache.LocalRegion.basicGetValueInVM(LocalRegion.java:3436) at org.apache.geode.internal.cache.LocalRegion.getValueInVM(LocalRegion.java:3424) at org.apache.geode.internal.cache.PartitionedRegionDataStore.getLocalValueInVM(PartitionedRegionDataStore.java:2775) at org.apache.geode.internal.cache.PartitionedRegion.getValueInVM(PartitionedRegion.java:8786) at org.apache.geode.internal.cache.SizingFlagDUnitTest$12.run(SizingFlagDUnitTest.java:797)
Re: Avoiding Unpredictability in Our DUnit Testing Rules
While it's common to use VM.getVM(int) to get a VM you still must use Host.getHost() to get a VM running an older version of Geode. Until someone adds a VM.getVM(version, vmNumber) we can't frown upon Host.getHost(0).getVM(startingVersion, 0). On 11/21/18 4:09 PM, Mark Hanson wrote: It is frowned upon. VM.getVM(0) is now the accepted way to get VM 0. Thanks, Mark On Nov 21, 2018, at 10:41 AM, Nabarun Nag wrote: Will doing this in the test, final Host host = Host.getHost(0); VM server1 = host.getVM(startingVersion, 0); be frowned upon, if I use the above over using @Rule. Regards Nabarun Nag On Nov 21, 2018, at 10:36 AM, Robert Houghton wrote: Great find, Patrick. I hope this shakes out some of the test bugs! On Wed, Nov 21, 2018, 10:34 Patrick Rhomberg tl;dr: Use JUnit RuleChain. Hello all! Several [1] of our test @Rule classes make use of the fact that our DUnit VMs Host is statically accessible to affect every test VM. For instance, the SharedCountersRule initializes a counter in every VM, and the CleanupDUnitVMsRule bounces VMs before and after each test. Problematically, JUnit rules applied in an unpredictable / JVM-dependent ordering. [2] As a result, some flakiness we find in our tests may be the result of unexpected interaction and ordering of our test rules. [3] Fortunately, a solution to this problem already exists. Rule ordering can be imposed by JUnit's RuleChain. [4] In early exploration with this rule, some tests failed due to the RuleChain not being serializable. However, as it should only apply to the test VM, and given that it can be composed of (unannotated) rules that remain accessible and serializable, it should be a simple matter of declaring the offending field transient, as it will only be necessary in the test VM. So, you dear reader: while you're out there making Geode the best it can be, if you find yourself in a test class that uses more than one rule listed in [1], or if you notice some other rule not listed below that reaches out to VMs as part of its @Before or @After, please update that test to use the RuleChain to apply the rules in a predictable order. Imagination is Change. ~Patrick [1] A probably-incomplete list of invasive rules can be found via $> git grep -il inEveryVM | grep Rule.java geode-core/src/distributedTest/java/org/apache/geode/management/ManagementTestRule.java geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/CacheRule.java geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/ClientCacheRule.java geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedDiskDirRule.java geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedRule.java geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedUseJacksonForJsonPathRule.java geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/SharedCountersRule.java [2] See the documentation for rules here: https://junit.org/junit4/javadoc/4.12/org/junit/Rule.html ; notably, "However, if there are multiple [Rule] fields (or methods) they will be applied in an order that depends on your JVM's implementation of the reflection API, which is undefined, in general." [3] For what it's worth, this was discovered after looking into why the DistributedRule check fo suspicious strings caused the test *after* the test that emitted the strings to fail. It's only tangentially related, but got me looking into when and how the @After was getting applied. [4] https://junit.org/junit4/javadoc/4.12/org/junit/rules/RuleChain.html
Re: Avoiding Unpredictability in Our DUnit Testing Rules
Is there a preferred ordering of the above list of rules? Or is the preferred ordering whichever passes? On Wed, Nov 21, 2018 at 4:36 PM Mark Hanson wrote: > It is frowned upon. VM.getVM(0) is now the accepted way to get VM 0. > > Thanks, > Mark > > > > On Nov 21, 2018, at 10:41 AM, Nabarun Nag wrote: > > > > Will doing this in the test, > > > > final Host host = Host.getHost(0); > > VM server1 = host.getVM(startingVersion, 0); > > > > be frowned upon, if I use the above over using @Rule. > > > > Regards > > Nabarun Nag > > > >> On Nov 21, 2018, at 10:36 AM, Robert Houghton > wrote: > >> > >> Great find, Patrick. I hope this shakes out some of the test bugs! > >> > >> On Wed, Nov 21, 2018, 10:34 Patrick Rhomberg wrote: > >> > >>> tl;dr: Use JUnit RuleChain. > >>> > >>> > >>> Hello all! > >>> > >>> Several [1] of our test @Rule classes make use of the fact that our > DUnit > >>> VMs Host is statically accessible to affect every test VM. For > instance, > >>> the SharedCountersRule initializes a counter in every VM, and the > >>> CleanupDUnitVMsRule bounces VMs before and after each test. > >>> > >>> Problematically, JUnit rules applied in an unpredictable / > JVM-dependent > >>> ordering. [2] As a result, some flakiness we find in our tests may be > the > >>> result of unexpected interaction and ordering of our test rules. [3] > >>> > >>> Fortunately, a solution to this problem already exists. Rule ordering > can > >>> be imposed by JUnit's RuleChain. [4] > >>> > >>> In early exploration with this rule, some tests failed due to the > RuleChain > >>> not being serializable. However, as it should only apply to the test > VM, > >>> and given that it can be composed of (unannotated) rules that remain > >>> accessible and serializable, it should be a simple matter of declaring > the > >>> offending field transient, as it will only be necessary in the test VM. > >>> > >>> So, you dear reader: while you're out there making Geode the best it > can > >>> be, if you find yourself in a test class that uses more than one rule > >>> listed in [1], or if you notice some other rule not listed below that > >>> reaches out to VMs as part of its @Before or @After, please update that > >>> test to use the RuleChain to apply the rules in a predictable order. > >>> > >>> Imagination is Change. > >>> ~Patrick > >>> > >>> [1] A probably-incomplete list of invasive rules can be found via > >>> $> git grep -il inEveryVM | grep Rule.java > >>> > >>> > geode-core/src/distributedTest/java/org/apache/geode/management/ManagementTestRule.java > >>> > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/CacheRule.java > >>> > >>> > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/ClientCacheRule.java > >>> > >>> > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedDiskDirRule.java > >>> > >>> > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedRule.java > >>> > >>> > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/DistributedUseJacksonForJsonPathRule.java > >>> > >>> > geode-dunit/src/main/java/org/apache/geode/test/dunit/rules/SharedCountersRule.java > >>> > >>> [2] See the documentation for rules here: > >>> https://junit.org/junit4/javadoc/4.12/org/junit/Rule.html ; notably, > >>> "However, > >>> if there are multiple [Rule] fields (or methods) they will be applied > in an > >>> order that depends on your JVM's implementation of the reflection API, > >>> which is undefined, in general." > >>> > >>> [3] For what it's worth, this was discovered after looking into why the > >>> DistributedRule check fo suspicious strings caused the test *after* the > >>> test that emitted the strings to fail. It's only tangentially > related, but > >>> got me looking into when and how the @After was getting applied. > >>> > >>> [4] > https://junit.org/junit4/javadoc/4.12/org/junit/rules/RuleChain.html > >>> > > > >