Skip to content

Loggers serialization safety - #2017

Merged
lossyrob merged 1 commit into
locationtech:masterfrom
pomadchin:fix/spark-persistent
Feb 21, 2017
Merged

lossyrob merged 1 commit into
locationtech:masterfrom
pomadchin:fix/spark-persistent

Conversation

@pomadchin

Copy link
Copy Markdown
Member

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 [email protected]

@pomadchin
pomadchin force-pushed the fix/spark-persistent branch from 9265d3f to 5cb43cb Compare February 16, 2017 16:44
@pomadchin pomadchin changed the title Logging safety Logging serialization safety Feb 16, 2017
@pomadchin pomadchin changed the title Logging serialization safety Loggers serialization safety Feb 16, 2017
@hjaekel

hjaekel commented Feb 21, 2017

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

Thanks for looking into that @hjaekel.

👍

@lossyrob
lossyrob merged commit 5513313 into locationtech:master Feb 21, 2017
@lossyrob lossyrob added this to the 1.1 milestone Mar 12, 2017
@echeipesh

Copy link
Copy Markdown
Contributor

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

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.

4 participants