Skip to content

Conversation

@GJL
Copy link
Member

@GJL GJL commented Feb 21, 2018

What is the purpose of the change

For the JSON representation, do not only serialize the serialized exception but the entire SerializedThrowable object. This makes it possible to throw the SerializedThrowable itself without deserializing it.

cc: @tillrohrmann

Brief change log

  • Change JSON serialization of SerializedThrowable.

Verifying this change

This change added tests and can be verified as follows:

  • Added unit tests for SerializedThrowableSerializer. Previously, only JobExecutionResultResponseBodyTest covered it.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

Do not only serialize the serialized exception but the entire
SerializedThrowable object. This makes it possible to throw the
SerializedThrowable itself without deserializing it.
/**
* Tests for {@link SerializedThrowableSerializer} and {@link SerializedThrowableDeserializer}.
*/
public class SerializedThrowableSerializerTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing TestLogger.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😞 I always forget that

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @GJL. Changes look good to me. Merging this PR.

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Feb 28, 2018
Do not only serialize the serialized exception but the entire
SerializedThrowable object. This makes it possible to throw the
SerializedThrowable itself without deserializing it.

This closes apache#5546.
tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Feb 28, 2018
Do not only serialize the serialized exception but the entire
SerializedThrowable object. This makes it possible to throw the
SerializedThrowable itself without deserializing it.

This closes apache#5546.
asfgit pushed a commit that referenced this pull request Feb 28, 2018
Do not only serialize the serialized exception but the entire
SerializedThrowable object. This makes it possible to throw the
SerializedThrowable itself without deserializing it.

This closes #5546.
@asfgit asfgit closed this in 970d94e Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants