Skip to content

Conversation

@levkhomich
Copy link
Contributor

A simple try-catch wrapping KryoException to be more informative.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@srowen
Copy link
Member

srowen commented Mar 9, 2015

ok to test

@srowen
Copy link
Member

srowen commented Mar 9, 2015

LGTM.

@SparkQA
Copy link

SparkQA commented Mar 9, 2015

Test build #28388 has started for PR 4947 at commit 48ab7f9.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 9, 2015

Test build #28388 has finished for PR 4947 at commit 48ab7f9.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28388/
Test PASSed.

Copy link
Member

Choose a reason for hiding this comment

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

Original message (Available and requested size) in KryoException is useful too. Is it better to include original message too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original exception is preserved as cause, so it is printed anyway.

Copy link
Member

Choose a reason for hiding this comment

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

But as the Exception's Constructor Detail (http://docs.oracle.com/javase/7/docs/api/java/lang/Exception.html#Exception(java.lang.String,%20java.lang.Throwable) states,

Note that the detail message associated with cause is not automatically incorporated in this exception's detail message.

Is it sure that it will be printed?

Copy link
Member

Choose a reason for hiding this comment

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

The cause stack trace / message would be printed by printStackTrace. It would not become part of the message from this new SparkException. Net-net I think it wouldn't hurt to just add additional info to the new SparkException message if it's deemed useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, you can check example of stack trace here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen @viirya I've squashed corresponding change.

@SparkQA
Copy link

SparkQA commented Mar 9, 2015

Test build #28397 has started for PR 4947 at commit 0f7a947.

  • This patch merges cleanly.

@srowen
Copy link
Member

srowen commented Mar 9, 2015

LGTM. I'll wait a bit longer for more comments.

@SparkQA
Copy link

SparkQA commented Mar 9, 2015

Test build #28397 has finished for PR 4947 at commit 0f7a947.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BinaryClassificationMetrics(JavaModelWrapper):

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28397/
Test PASSed.

@sryza
Copy link
Contributor

sryza commented Mar 9, 2015

Is this not needed for serializeStream as well?

@levkhomich
Copy link
Contributor Author

@sryza KryoSerializer.maxBufferSize is used only in KryoSerializer.newKryoOutput, which, in turn, is used only in KryoSerializerInstance. KryoSerializationStream uses output passed in arguments, so spark.kryoserializer.buffer.max.mb doesn't affect it. Same applies to JavaIterableWrapperSerializer.

@srowen
Copy link
Member

srowen commented Mar 9, 2015

Yes, that looks correct. In the other cases, we aren't writing to an internal buffer, not one that is controlled by this buffer size setting.

@asfgit asfgit closed this in c4c4b07 Mar 10, 2015
asfgit pushed a commit that referenced this pull request Mar 13, 2015
… large enough

A simple try-catch wrapping KryoException to be more informative.

Author: Lev Khomich <levkhomich@gmail.com>

Closes #4947 from levkhomich/master and squashes the following commits:

0f7a947 [Lev Khomich] [SPARK-6087][CORE] Provide actionable exception if Kryo buffer is not large enough
superbobry added a commit to superbobry/spark that referenced this pull request Dec 27, 2016
This is to workaround an implicit result of apache#4947 which suppressed the
original Kryo exception if the overflow happened during serialization.
ghost pushed a commit to dbtsai/spark that referenced this pull request Dec 28, 2016
## What changes were proposed in this pull request?

This is to workaround an implicit result of apache#4947 which suppressed the
original Kryo exception if the overflow happened during serialization.

## How was this patch tested?

`KryoSerializerSuite` was augmented to reflect this change.

Author: Sergei Lebedev <superbobry@gmail.com>

Closes apache#16416 from superbobry/patch-1.
superbobry added a commit to superbobry/spark that referenced this pull request Dec 28, 2016
This is to workaround an implicit result of apache#4947 which suppressed the
original Kryo exception if the overflow happened during serialization.
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Dec 29, 2016
## What changes were proposed in this pull request?

This is to workaround an implicit result of apache#4947 which suppressed the
original Kryo exception if the overflow happened during serialization.

## How was this patch tested?

`KryoSerializerSuite` was augmented to reflect this change.

Author: Sergei Lebedev <superbobry@gmail.com>

Closes apache#16416 from superbobry/patch-1.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?

This is to workaround an implicit result of apache#4947 which suppressed the
original Kryo exception if the overflow happened during serialization.

## How was this patch tested?

`KryoSerializerSuite` was augmented to reflect this change.

Author: Sergei Lebedev <superbobry@gmail.com>

Closes apache#16416 from superbobry/patch-1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants