mundaym commented on a change in pull request #29407:
URL: https://github.com/apache/spark/pull/29407#discussion_r473065960
##########
File path: core/src/test/scala/org/apache/spark/storage/MemoryStoreSuite.scala
##########
@@ -51,12 +52,34 @@ class MemoryStoreSuite
implicit def StringToBlockId(value: String): BlockId = new TestBlockId(value)
def rdd(rddId: Int, splitId: Int): RDDBlockId = RDDBlockId(rddId, splitId)
+ // Save modified system properties so that we can restore them after tests.
+ val originalArch = System.getProperty("os.arch")
+ val originalCompressedOops = System.getProperty(TEST_USE_COMPRESSED_OOPS_KEY)
+
+ def reinitializeSizeEstimator(arch: String, useCompressedOops: String): Unit
= {
+ val set = (k: String, v: String) => {
+ if (v == null) {
+ System.clearProperty(k)
+ } else {
+ System.setProperty(k, v)
+ }
+ }
+ set("os.arch", arch)
+ set(TEST_USE_COMPRESSED_OOPS_KEY, useCompressedOops)
+ val initialize = PrivateMethod[Unit](Symbol("initialize"))
+ SizeEstimator invokePrivate initialize()
+ }
+
override def beforeEach(): Unit = {
super.beforeEach()
// Set the arch to 64-bit and compressedOops to true to get a
deterministic test-case
- System.setProperty("os.arch", "amd64")
- val initialize = PrivateMethod[Unit](Symbol("initialize"))
- SizeEstimator invokePrivate initialize()
+ reinitializeSizeEstimator("amd64", "true")
Review comment:
Yeah, this test suite assumes that compressed oops are enabled. The
comment above alludes to this but the original code didn't actually override
it. This leads to test failures when running a JVM where compressed oops are
disabled.
In another test ("64-bit arch with no compressed oops") the test was
inheriting the compressed oops setting of false from a preceding test rather
than setting it itself. Since the preceding test is now cleaning up after
itself that test now explicitly needs to set compressed oops to false.
In general it seems better to explicitly set these settings. Or we could try
and make the tests more robust to different arch and compressed oops
combinations. That is a bit more involved than this patch though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
