[SPARK-7180][SPARK-8090][SPARK-8091] Fix a number of SerializationDebugger bugs and limitations #6625

tdas · 2015-06-03T21:59:03Z

This PR solves three SerializationDebugger issues.

SPARK-7180 - SerializationDebugger fails with ArrayOutOfBoundsException
SPARK-8090 - SerializationDebugger does not handle classes with writeReplace correctly
SPARK-8091 - SerializationDebugger does not handle classes with writeObject method

The solutions for each are explained as follows

SPARK-7180 - The wrong slot desc was used for getting the value of the fields in the object being tested.
SPARK-8090 - Test the type of the replaced object.
SPARK-8091 - Use a dummy ObjectOutputStream to collect all the objects written by the writeObject() method, and then test those objects as usual.

I also added more tests in the testsuite to increase code coverage. For example, added tests for cases where there are not serializability issues.

tdas · 2015-06-03T22:00:04Z

@andrewor14 Please take a look.
I am still adding more comments, so this is sort-of WIP.

tdas · 2015-06-03T22:03:49Z

@rxin Take a look. Adding comments, so slightly WIP.

tdas · 2015-06-03T22:04:21Z

core/src/main/scala/org/apache/spark/serializer/SerializationDebugger.scala

        } else {
          val fields: Array[ObjectStreamField] = slotDesc.getFields
          val objFieldValues: Array[Object] = new Array[Object](slotDesc.getNumObjFields)
          val numPrims = fields.length - objFieldValues.length
-          desc.getObjFieldValues(finalObj, objFieldValues)
+          slotDesc.getObjFieldValues(finalObj, objFieldValues)


This is the bug fix for SPARK-7180

andrewor14 · 2015-06-03T22:33:21Z

core/src/main/scala/org/apache/spark/serializer/SerializationDebugger.scala

+
+    private def visitSerializableWithWriteObjectMethod(
+        o: Object, slotDesc: ObjectStreamClass, stack: List[String]): List[String] = {
+      println(">>> processing serializable with writeObject" + o)


stray println

Already removed, waiting for more feedback to accumulate before pushing.

andrewor14 · 2015-06-03T22:37:30Z

As discussed offline, this looks great. I am looking forward to your land mine of comments...

SparkQA · 2015-06-04T00:04:59Z

Test build #34125 has finished for PR 6625 at commit da75d34.

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

SparkQA · 2015-06-04T00:52:24Z

Test build #34139 has finished for PR 6625 at commit 7e2fdcf.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends Parent(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

tdas · 2015-06-04T01:22:27Z

@andrewor14 How about them comments? Also added more tests. 93% line coverage.

SparkQA · 2015-06-04T01:29:51Z

Test build #34145 has finished for PR 6625 at commit 26b5179.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends Parent(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

tdas · 2015-06-04T01:42:20Z

core/src/main/scala/org/apache/spark/serializer/SerializationDebugger.scala

@@ -62,7 +62,7 @@ private[spark] object SerializationDebugger extends Logging {
   *
   * It does not yet handle writeObject override, but that shouldn't be too hard to do either.
   */
-  def find(obj: Any): List[String] = {
+  private[serializer] def find(obj: Any): List[String] = {


so that everyone uses improveException which is safe to call.

SparkQA · 2015-06-04T02:28:57Z

Test build #34138 has finished for PR 6625 at commit d1967fb.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends Parent(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

andrewor14 · 2015-06-04T02:36:47Z

core/src/main/scala/org/apache/spark/serializer/SerializationDebugger.scala

+     * Visit an externalizable object.
+     * Since writeExternal() can choose add arbitrary objects at the time of serialization,
+     * the only way to capture all the objects it will serialize is by using a
+     * dummy ObjectOutput object that captures all the inner objects, and then visit all the


This is a really long sentence and fairly low level. How about:

Since writeExternal() can choose add arbitrary objects at the time of serialization, the only way to capture all the objects it will serialize is to manually write the object into our own dummy ObjectOutput that collects the relevant fields.

Okay, I can shorten it for this, as well as for the new visit function.

SparkQA · 2015-06-04T03:39:29Z

Test build #34146 has finished for PR 6625 at commit 304c97b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends Parent(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

SparkQA · 2015-06-04T06:47:02Z

Test build #34154 has finished for PR 6625 at commit ae212c8.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends ParentClass(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

andrewor14 · 2015-06-04T23:32:19Z

core/src/main/scala/org/apache/spark/serializer/SerializationDebugger.scala

+      }
+
+      // Every class is associated with one or more "slots", each slot is related to the parent
+      // classes of this class. These slots are used by the ObjectOutputStream


grammar nit: each slot refers to the parent class of this class

andrewor14 · 2015-06-04T23:42:04Z

I left a bunch of english nits, but this is mergeable as is. LGTM.

JoshRosen · 2015-06-13T19:28:48Z

Bump @tdas @andrewor14, do we want to merge this now or should we wait for the comment changes? We may want to retest with Jenkins first, just in case there's any new style rules that this might fail.

andrewor14 · 2015-06-13T23:10:48Z

retest this please

SparkQA · 2015-06-14T00:59:17Z

Test build #34851 has finished for PR 6625 at commit ae212c8.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends ParentClass(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

JoshRosen · 2015-06-18T06:19:52Z

Gentle bump again: do we want to merge this as-is and fix the comments later?

andrewor14 · 2015-06-18T19:38:51Z

@tdas is working on this right now I believe

SparkQA · 2015-06-18T22:02:40Z

Test build #35156 has finished for PR 6625 at commit c7cb046.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- // class ParentClass(parentField: Int)
- // class ChildClass(childField: Int) extends ParentClass(1)
- // If the class type corresponding to current slot has writeObject() defined,
- // then its not obvious which fields of the class will be serialized as the writeObject()

tdas · 2015-06-18T23:02:24Z

@andrewor14 This is good to go.

andrewor14 · 2015-06-19T03:06:25Z

Ok, merging into master.

andrewor14 · 2015-06-19T18:06:47Z

I also merged this into 1.4.1.

…ebugger bugs and limitations This PR solves three SerializationDebugger issues. * SPARK-7180 - SerializationDebugger fails with ArrayOutOfBoundsException * SPARK-8090 - SerializationDebugger does not handle classes with writeReplace correctly * SPARK-8091 - SerializationDebugger does not handle classes with writeObject method The solutions for each are explained as follows * SPARK-7180 - The wrong slot desc was used for getting the value of the fields in the object being tested. * SPARK-8090 - Test the type of the replaced object. * SPARK-8091 - Use a dummy ObjectOutputStream to collect all the objects written by the writeObject() method, and then test those objects as usual. I also added more tests in the testsuite to increase code coverage. For example, added tests for cases where there are not serializability issues. Author: Tathagata Das <[email protected]> Closes #6625 from tdas/SPARK-7180 and squashes the following commits: c7cb046 [Tathagata Das] Addressed comments on docs ae212c8 [Tathagata Das] Improved docs 304c97b [Tathagata Das] Fixed build error 26b5179 [Tathagata Das] more tests.....92% line coverage 7e2fdcf [Tathagata Das] Added more tests d1967fb [Tathagata Das] Added comments. da75d34 [Tathagata Das] Removed unnecessary lines. 50a608d [Tathagata Das] Fixed bugs and added support for writeObject

…ebugger bugs and limitations This PR solves three SerializationDebugger issues. * SPARK-7180 - SerializationDebugger fails with ArrayOutOfBoundsException * SPARK-8090 - SerializationDebugger does not handle classes with writeReplace correctly * SPARK-8091 - SerializationDebugger does not handle classes with writeObject method The solutions for each are explained as follows * SPARK-7180 - The wrong slot desc was used for getting the value of the fields in the object being tested. * SPARK-8090 - Test the type of the replaced object. * SPARK-8091 - Use a dummy ObjectOutputStream to collect all the objects written by the writeObject() method, and then test those objects as usual. I also added more tests in the testsuite to increase code coverage. For example, added tests for cases where there are not serializability issues. Author: Tathagata Das <[email protected]> Closes apache#6625 from tdas/SPARK-7180 and squashes the following commits: c7cb046 [Tathagata Das] Addressed comments on docs ae212c8 [Tathagata Das] Improved docs 304c97b [Tathagata Das] Fixed build error 26b5179 [Tathagata Das] more tests.....92% line coverage 7e2fdcf [Tathagata Das] Added more tests d1967fb [Tathagata Das] Added comments. da75d34 [Tathagata Das] Removed unnecessary lines. 50a608d [Tathagata Das] Fixed bugs and added support for writeObject

tdas changed the title ~~[SPARK-7180][SPARK-8090][SPARK-8191]~~ [SPARK-7180][SPARK-8090][SPARK-8191] Fix a number of SerializableDebugger bugs and limitations Jun 3, 2015

tdas changed the title ~~[SPARK-7180][SPARK-8090][SPARK-8191] Fix a number of SerializableDebugger bugs and limitations~~ [SPARK-7180][SPARK-8090][SPARK-8191] Fix a number of SerializationDebugger bugs and limitations Jun 3, 2015

Fixed bugs and added support for writeObject

50a608d

tdas force-pushed the SPARK-7180 branch from a24535b to 50a608d Compare June 3, 2015 22:01

tdas reviewed Jun 3, 2015
View reviewed changes

Removed unnecessary lines.

da75d34

andrewor14 reviewed Jun 3, 2015
View reviewed changes

tdas added 2 commits June 3, 2015 17:34

Added comments.

d1967fb

Added more tests

7e2fdcf

more tests.....92% line coverage

26b5179

Fixed build error

304c97b

tdas reviewed Jun 4, 2015
View reviewed changes

tdas changed the title ~~[SPARK-7180][SPARK-8090][SPARK-8191] Fix a number of SerializationDebugger bugs and limitations~~ [SPARK-7180][SPARK-8090][SPARK-8091] Fix a number of SerializationDebugger bugs and limitations Jun 4, 2015

andrewor14 reviewed Jun 4, 2015
View reviewed changes

Improved docs

ae212c8

andrewor14 reviewed Jun 4, 2015
View reviewed changes

Addressed comments on docs

c7cb046

asfgit closed this in 866816e Jun 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7180][SPARK-8090][SPARK-8091] Fix a number of SerializationDebugger bugs and limitations #6625

[SPARK-7180][SPARK-8090][SPARK-8091] Fix a number of SerializationDebugger bugs and limitations #6625

tdas commented Jun 3, 2015

tdas commented Jun 3, 2015

tdas commented Jun 3, 2015

tdas Jun 3, 2015

andrewor14 Jun 3, 2015

tdas Jun 3, 2015

andrewor14 commented Jun 3, 2015

SparkQA commented Jun 4, 2015

SparkQA commented Jun 4, 2015

tdas commented Jun 4, 2015

SparkQA commented Jun 4, 2015

tdas Jun 4, 2015

SparkQA commented Jun 4, 2015

andrewor14 Jun 4, 2015

tdas Jun 4, 2015

SparkQA commented Jun 4, 2015

SparkQA commented Jun 4, 2015

andrewor14 Jun 4, 2015

andrewor14 commented Jun 4, 2015

JoshRosen commented Jun 13, 2015

andrewor14 commented Jun 13, 2015

SparkQA commented Jun 14, 2015

JoshRosen commented Jun 18, 2015

andrewor14 commented Jun 18, 2015

SparkQA commented Jun 18, 2015

tdas commented Jun 18, 2015

andrewor14 commented Jun 19, 2015

andrewor14 commented Jun 19, 2015

[SPARK-7180][SPARK-8090][SPARK-8091] Fix a number of SerializationDebugger bugs and limitations #6625

[SPARK-7180][SPARK-8090][SPARK-8091] Fix a number of SerializationDebugger bugs and limitations #6625

Conversation

tdas commented Jun 3, 2015

tdas commented Jun 3, 2015

tdas commented Jun 3, 2015

tdas Jun 3, 2015

Choose a reason for hiding this comment

andrewor14 Jun 3, 2015

Choose a reason for hiding this comment

tdas Jun 3, 2015

Choose a reason for hiding this comment

andrewor14 commented Jun 3, 2015

SparkQA commented Jun 4, 2015

SparkQA commented Jun 4, 2015

tdas commented Jun 4, 2015

SparkQA commented Jun 4, 2015

tdas Jun 4, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 4, 2015

andrewor14 Jun 4, 2015

Choose a reason for hiding this comment

tdas Jun 4, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 4, 2015

SparkQA commented Jun 4, 2015

andrewor14 Jun 4, 2015

Choose a reason for hiding this comment

andrewor14 commented Jun 4, 2015

JoshRosen commented Jun 13, 2015

andrewor14 commented Jun 13, 2015

SparkQA commented Jun 14, 2015

JoshRosen commented Jun 18, 2015

andrewor14 commented Jun 18, 2015

SparkQA commented Jun 18, 2015

tdas commented Jun 18, 2015

andrewor14 commented Jun 19, 2015

andrewor14 commented Jun 19, 2015