[SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue #24264

LantaoJin · 2019-04-01T14:16:10Z

What changes were proposed in this pull request?

HighlyCompressedMapStatus uses RoaringBitmap to record the empty blocks. But RoaringBitmap couldn't be ser/deser with unsafe KryoSerializer.

It's a bug of RoaringBitmap-0.5.11 and fixed in latest version.

This is an update of #24157

How was this patch tested?

Add a UT

LantaoJin · 2019-04-01T14:19:28Z

@srowen @squito Could you help to review this PR? I can not reopen #24157 since a force pushing. Thanks for your time.

srowen · 2019-04-01T14:23:19Z

core/src/test/scala/org/apache/spark/serializer/UnsafeKryoSerializerSuite.scala

+
+    conf.set(KRYO_USE_UNSAFE, false)
+    val safeSer = new KryoSerializer(conf).newInstance()
+    var actual : RoaringBitmap = safeSer.deserialize(safeSer.serialize(expected))


Nit: I think the style checker will flag the space before the colon.

srowen · 2019-04-01T14:23:29Z

core/src/test/scala/org/apache/spark/serializer/UnsafeKryoSerializerSuite.scala

  }
+
+  test("SPARK-27216: Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue") {
+    val expected = new RoaringBitmap


Tiny nit: () after the constructor

SparkQA · 2019-04-01T14:27:06Z

Test build #4675 has finished for PR 24264 at commit 076411b.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

LantaoJin · 2019-04-01T14:31:07Z

Rename for a better searching or tracing.

attilapiros · 2019-04-01T14:35:31Z

core/src/test/scala/org/apache/spark/serializer/UnsafeKryoSerializerSuite.scala

    super.afterAll()
  }
+
+  test("SPARK-27216: Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue") {


Please move your test into KryoSerializerSuite where conf.set(KRYO_USE_UNSAFE, false) is given.
And as UnsafeKryoSerializerSuite is inherited from KryoSerializerSuite beside using conf.set(KRYO_USE_UNSAFE, true) in beforeAll() it will inherit your test method so both the safe/unsafe case will be tested this way: once executing KryoSerializerSuite and once UnsafeKryoSerializerSuite.

Summary: you do not need to test both cases in one test method.

Oh, thanks for this reminding.

srowen · 2019-04-01T14:41:27Z

Run ./dev/test-dependencies.sh --replace-manifest

squito · 2019-04-02T04:15:34Z

core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala

+    val expected = new RoaringBitmap()
+    expected.add(1787)
+
+    conf.set(KRYO_USE_UNSAFE, true)


I dont' think this is what @attilapiros meant. I think he meant to (a) leave UnsafeKryoSerializerSuite as it was before and (b) add a new test which just tested serialization of RoaringBitmap with kryo in this class, but didn't tweak confs at all. That way you'd get a test for serializing with kryo.unsafe=false (here in KryoSerializerSuite) and you'd also automatically get a test with kryo.unsafe=true (inherited in UnsafeKryoSerializerSuite).

Exactly. I was about to write a similar comment.

LantaoJin · 2019-04-02T09:11:53Z

Uh... @attilapiros Is it proper now?

attilapiros · 2019-04-02T09:22:08Z

@LantaoJin No.

revert the deletion of the original UnsafeKryoSerializerSuite.scala

remove these lines:

spark/core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala

Lines 531 to 543 in f0f62af

    
           class UnsafeKryoSerializerSuite extends SparkFunSuite with SharedSparkContext { 
        
             conf.set(SERIALIZER, classOf[KryoSerializer].getName) 
        
             conf.set(KRYO_USER_REGISTRATORS, classOf[MyRegistrator].getName) 
        
             conf.set(KRYO_USE_UNSAFE, true) 
        
             test("test RoaringBitmap ser/dser with Kryo unsafe enabled") { 
        
               val expected = new RoaringBitmap() 
        
               expected.add(1787) 
        
               val unsafeSer = new KryoSerializer(conf).newInstance() 
        
               val actual: RoaringBitmap = unsafeSer.deserialize(unsafeSer.serialize(expected)) 
        
               assert(actual === expected) 
        
             } 
        
           }

attilapiros · 2019-04-02T09:28:01Z

Finally via sbt test both UnsafeKryoSerializerSuite and KryoSerializerSuite and you should see your RoaringBitmap test executed twice, you can do that like this:

$ ./build/sbt
> project core
> testOnly *.UnsafeKryoSerializerSuite
> testOnly *.KryoSerializerSuite

attilapiros · 2019-04-02T09:36:52Z

You can revert the deletion by:

git checkout f799e34962c24bb55662e7e570195d8f5aa5799a core/src/test/scala/org/apache/spark/serializer/UnsafeKryoSerializerSuite.scala

But please double check the commit ID (my laptop used for developing Spark runs an OS update and this is my other machine...).

LantaoJin · 2019-04-02T09:37:53Z

Finally via sbt test both UnsafeKryoSerializerSuite and KryoSerializerSuite and you should see your RoaringBitmap test executed twice, you can do that like this:
$ ./build/sbt
> project core
> testOnly *.UnsafeKryoSerializerSuite
> testOnly *.KryoSerializerSuite

Everything looks fine from my side.
> testOnly *.UnsafeKryoSerializerSuite
[info] UnsafeKryoSerializerSuite:
[info] - test RoaringBitmap ser/dser with Kryo unsafe enabled (264 milliseconds)
[info] ScalaTest
[info] Run completed in 4 seconds, 430 milliseconds.
[info] Total number of tests run: 1
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 1, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[info] Passed: Total 1, Failed 0, Errors 0, Passed 1
[success] Total time: 97 s, completed Apr 2, 2019 5:32:05 PM
> testOnly *.KryoSerializerSuite
[warn] Multiple main classes detected. Run 'show discoveredMainClasses' to see the list
[info] Packaging /Users/lajin/git/my/spark/core/target/scala-2.12/spark-core_2.12-3.0.0-SNAPSHOT.jar ...
[info] Done packaging.
[info] KryoSerializerSuite:
[info] - SPARK-7392 configuration limits (22 milliseconds)
....
[info] - test RoaringBitmap ser/dser with Kryo unsafe disabled (6 milliseconds)
[info] ScalaTest
[info] Run completed in 5 seconds, 503 milliseconds.
[info] Total number of tests run: 34
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 34, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[info] Passed: Total 34, Failed 0, Errors 0, Passed 34

LantaoJin · 2019-04-02T09:44:48Z

@attilapiros Oh, I read your comment again. I guess I have understood it. I will refactor it.

LantaoJin · 2019-04-02T09:53:20Z

@attilapiros What about it now?

attilapiros · 2019-04-02T12:10:03Z

@LantaoJin Now your change is almost perfect.

But as there was no need to import RoaringBitmap for your test it made me think what kind of tests were here before your PR regarding this class.

I have found this:

spark/core/src/test/scala/org/apache/spark/serializer/KryoSerializerSuite.scala

Lines 373 to 396 in 8cc3eb4

    
           test("SPARK-12222: deserialize RoaringBitmap throw Buffer underflow exception") { 
        
             withTempDir { dir => 
        
               val tmpfile = dir.toString + "/RoaringBitmap" 
        
               val outStream = new FileOutputStream(tmpfile) 
        
               val output = new KryoOutput(outStream) 
        
               val bitmap = new RoaringBitmap 
        
               bitmap.add(1) 
        
               bitmap.add(3) 
        
               bitmap.add(5) 
        
               // Ignore Kryo because it doesn't use writeObject 
        
               bitmap.serialize(new KryoOutputObjectOutputBridge(null, output)) 
        
               output.flush() 
        
               output.close() 
        
               val inStream = new FileInputStream(tmpfile) 
        
               val input = new KryoInput(inStream) 
        
               val ret = new RoaringBitmap 
        
               // Ignore Kryo because it doesn't use readObject 
        
               ret.deserialize(new KryoInputObjectInputBridge(null, input)) 
        
               input.close() 
        
               assert(ret == bitmap) 
        
             } 
        
           }

I think it is out-dated and this test can be removed as the statement deserialize RoaringBitmap throw Buffer underflow exception is false, what do you think?

LantaoJin · 2019-04-02T12:35:27Z

I think it's out-dated. But it needs double check if we plan to merge this PR to multiple branches.

attilapiros · 2019-04-02T12:44:14Z

As this change contradicts with that statement it is safe to remove that test.

LantaoJin · 2019-04-02T13:05:24Z

I've checked branch 2.1 and above. The kryo versions in them are all 3.0 or above. According to the description in SPARK-12222, it should be safe. I will remove it.

attilapiros · 2019-04-02T13:09:52Z

Jenkins retest this please.

squito · 2019-04-02T13:18:55Z

lgtm assuming tests pass. Thanks for the work on this @LantaoJin & @attilapiros

SparkQA · 2019-04-02T18:17:11Z

Test build #4679 has finished for PR 24264 at commit 3397957.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

lemire · 2019-04-03T22:04:32Z

You might want to consider upgrading directly to RoaringBitmap 0.8.0. We added serialize(ByteBuffer) and deserialize(ByteBuffer) methods that are quite interesting (and fast).

cc @richardstartin @blacelle

squito · 2019-04-04T01:08:29Z

@lemire I think that's a good idea for something to do eventually, but I think that can be handled separately. IIUC, to take advantage of those performance improvements, we need to change how we're serializing the RoaringBitmaps inside spark. This is a bug fix we can get in now.

I filed https://issues.apache.org/jira/browse/SPARK-27367 for this if anybody wants to follow up with that

squito · 2019-04-04T01:11:50Z

I merged this to master, but there were merge conflicts going back farther. @LantaoJin would you like to open another pr, against branch 2.4 at least?

…fix Kryo unsafe ser/dser issue ## What changes were proposed in this pull request? Back-port of #24264 to branch-2.4. HighlyCompressedMapStatus uses RoaringBitmap to record the empty blocks. But RoaringBitmap couldn't be ser/deser with unsafe KryoSerializer. It's a bug of RoaringBitmap-0.5.11 and fixed in latest version. ## How was this patch tested? Add a UT Closes #24290 from LantaoJin/SPARK-27216_BACKPORT-2.4. Authored-by: LantaoJin <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…fix Kryo unsafe ser/dser issue ## What changes were proposed in this pull request? Back-port of #24264 to branch-2.3. HighlyCompressedMapStatus uses RoaringBitmap to record the empty blocks. But RoaringBitmap couldn't be ser/deser with unsafe KryoSerializer. It's a bug of RoaringBitmap-0.5.11 and fixed in latest version. ## How was this patch tested? Add a UT Closes #24291 from LantaoJin/SPARK-27216_BACKPORT-2.3. Authored-by: LantaoJin <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…fix Kryo unsafe ser/dser issue ## What changes were proposed in this pull request? Back-port of apache#24264 to branch-2.4. HighlyCompressedMapStatus uses RoaringBitmap to record the empty blocks. But RoaringBitmap couldn't be ser/deser with unsafe KryoSerializer. It's a bug of RoaringBitmap-0.5.11 and fixed in latest version. ## How was this patch tested? Add a UT Closes apache#24290 from LantaoJin/SPARK-27216_BACKPORT-2.4. Authored-by: LantaoJin <[email protected]> Signed-off-by: Sean Owen <[email protected]>

LantaoJin added 5 commits April 1, 2019 21:09

[SPARK-27216][CORE] Kryo serialization with RoaringBitmap

31c1b37

ut move to UnsafeKryoSerializerSuite

bafb560

add a sql related ut to illustrate

fcafcd4

trivial

cef8928

[SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45

076411b

LantaoJin mentioned this pull request Apr 1, 2019

[SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45 #24157

Closed

srowen reviewed Apr 1, 2019

View reviewed changes

address comment

2821247

address comment

47f0912

LantaoJin changed the title ~~[SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45~~ [SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue Apr 1, 2019

attilapiros reviewed Apr 1, 2019

View reviewed changes

address comment

af0a6bd

squito reviewed Apr 2, 2019

View reviewed changes

address comment

f0f62af

address comment

8cc3eb4

remove out-dated test

3397957

srowen approved these changes Apr 3, 2019

View reviewed changes

richardstartin mentioned this pull request Apr 3, 2019

serialize to bytebuffer RoaringBitmap/RoaringBitmap#325

Merged

asfgit closed this in 69dd44a Apr 4, 2019

This was referenced Apr 4, 2019

[SPARK-27216][CORE][BACKPORT-2.4] Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue #24290

Closed

[SPARK-27216][CORE][BACKPORT-2.3] Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue #24291

Closed

[SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue #24264

[SPARK-27216][CORE] Upgrade RoaringBitmap to 0.7.45 to fix Kryo unsafe ser/dser issue #24264

Uh oh!

Conversation

LantaoJin commented Apr 1, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

LantaoJin commented Apr 1, 2019

Uh oh!

srowen Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

srowen Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 1, 2019

Uh oh!

LantaoJin commented Apr 1, 2019

Uh oh!

attilapiros Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

srowen commented Apr 1, 2019

Uh oh!

squito Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

attilapiros Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

LantaoJin commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

attilapiros commented Apr 2, 2019

Uh oh!

attilapiros commented Apr 2, 2019

Uh oh!

attilapiros commented Apr 2, 2019

Uh oh!

LantaoJin commented Apr 2, 2019

Uh oh!

LantaoJin commented Apr 2, 2019

Uh oh!

LantaoJin commented Apr 2, 2019

Uh oh!

attilapiros commented Apr 2, 2019

Uh oh!

LantaoJin commented Apr 2, 2019

Uh oh!

attilapiros commented Apr 2, 2019

Uh oh!

LantaoJin commented Apr 2, 2019

Uh oh!

attilapiros commented Apr 2, 2019

Uh oh!

squito commented Apr 2, 2019

Uh oh!

SparkQA commented Apr 2, 2019

Uh oh!

lemire commented Apr 3, 2019

Uh oh!

squito commented Apr 4, 2019

Uh oh!

squito commented Apr 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

attilapiros Apr 1, 2019 •

edited

Loading

LantaoJin commented Apr 2, 2019 •

edited

Loading