New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loggers serialization safety #2017

Merged
merged 1 commit into from Feb 21, 2017

Conversation

Projects
None yet
4 participants
@pomadchin
Member

pomadchin commented Feb 16, 2017

Even though loggers are serializable, we have to be sure that there would be no problems during multiple ser / deser. Spark internal loggers all marked with @transient, all Hadoop loggers are final static. There is still no unit tests related to that issue, though you can notice it during Spark memory shuffle and using DISK_ONLY or MEMORY_AND_DISK RDD persistence levels.

Signed-off-by: Grigory Pomadchin gr.pomadchin@gmail.com

transient logging in all places we use it
Signed-off-by: Grigory Pomadchin <gr.pomadchin@gmail.com>

@pomadchin pomadchin force-pushed the pomadchin:fix/spark-persistent branch from 9265d3f to 5cb43cb Feb 16, 2017

@pomadchin pomadchin changed the title from Logging safety to Logging serialization safety Feb 16, 2017

@pomadchin pomadchin changed the title from Logging serialization safety to Loggers serialization safety Feb 16, 2017

@hjaekel

This comment has been minimized.

Contributor

hjaekel commented Feb 21, 2017

We tested your patch with our code and the NullPointerException is gone, everything works fine now.

Just for the documentation, this was the exception we get when GeoTrellis is using the com.typesafe.scalalogging.LazyLogging:

java.lang.NullPointerException
        at org.slf4j.impl.Log4jLoggerAdapter.isWarnEnabled(Log4jLoggerAdapter.java:391)
        at geotrellis.raster.ArrayTile$class.convert(ArrayTile.scala:46)
        at geotrellis.raster.UShortArrayTile.convert(UShortArrayTile.scala:24)

We could not reproduce this issue in unit tests, too. The NPEs just occur while executing bigger processes with limited memory.

@lossyrob

This comment has been minimized.

Member

lossyrob commented Feb 21, 2017

Thanks for looking into that @hjaekel.

👍

@lossyrob lossyrob merged commit 5513313 into locationtech:master Feb 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lossyrob lossyrob removed the in progress label Feb 21, 2017

@lossyrob lossyrob added the api change label Mar 12, 2017

@lossyrob lossyrob added this to the 1.1 milestone Mar 12, 2017

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Mar 13, 2017

I don't think this is a breaking change or even api-change. We replaced a protected field with a protected field.

And addition of @transient does not break binary compatibility: https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.11

@lossyrob lossyrob removed the api change label Mar 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment