Reading-eScience-Centre / edal-java

Environmental Data Abstraction Layer libraries
Other
39 stars 30 forks source link

Weird temporal bounds check issue when using default/recent Joda time librarry. #72

Closed julian1 closed 7 years ago

julian1 commented 7 years ago

I've created a simple project following the NcDiag.java example and using DiscreteLayeredDataset to examine netcdf files.

If I don't specify joda-time dependency in the pom - then it defaults to 2.8.1. And running my example generates the following joda bounds check error,

$ ./run.sh 
<h1>Report from /home/old-meteo/imos/projects/docker_ncwms_s3/IMOS/ACORN/gridded_1h-avg-current-map_QC/ROT/2014/01/02/IMOS_ACORN_V_20140102T053000Z_ROT_FV01_1-hour-avg.nc</h1>
log4j:WARN No appenders could be found for logger (ucar.nc2.NetcdfFile).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
<h2>File could not be read as a dataset:</h2>
<h2>/home/old-meteo/imos/projects/docker_ncwms_s3/IMOS/ACORN/gridded_1h-avg-current-map_QC/ROT/2014/01/02/IMOS_ACORN_V_20140102T053000Z_ROT_FV01_1-hour-avg.nc</h2>
<h2>Stack trace follows:</h2>

org.joda.time.IllegalFieldValueException: Value 292278994 for year must be in the range [-292275054,292278993]
        at org.joda.time.field.FieldUtils.verifyValueBounds(FieldUtils.java:234)
        at org.joda.time.chrono.BasicYearDateTimeField.set(BasicYearDateTimeField.java:83)
        at org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:129)
        at org.joda.time.DateTime.<init>(DateTime.java:236)
        at uk.ac.rdg.resc.edal.util.GISUtils.getIntersectionOfTemporalDomains(GISUtils.java:1069)
        at uk.ac.rdg.resc.edal.dataset.plugins.VariablePlugin.newVariableMetadataFromDomains(VariablePlugin.java:419)
        at uk.ac.rdg.resc.edal.dataset.plugins.VariablePlugin.newVariableMetadataFromMetadata(VariablePlugin.java:384)
        at uk.ac.rdg.resc.edal.dataset.plugins.VectorPlugin.doProcessVariableMetadata(VectorPlugin.java:121)
        at uk.ac.rdg.resc.edal.dataset.plugins.VariablePlugin.processVariableMetadata(VariablePlugin.java:220)
        at uk.ac.rdg.resc.edal.dataset.AbstractDataset.addVariablePlugin(AbstractDataset.java:174)
        at uk.ac.rdg.resc.edal.dataset.cdm.CdmDatasetFactory.createDataset(CdmDatasetFactory.java:97)
        at uk.ac.rdg.resc.edal.dataset.cdm.CdmDatasetFactory.createDataset(CdmDatasetFactory.java:72)
        at com.example.Reconciliation.diagnoseDataset(Reconciliation.java:100)
        at com.example.Reconciliation.main(Reconciliation.java:170)

Making a single change to the pom to specify version 2.2 joda dependency - to match https://github.com/Reading-eScience-Centre/edal-java/blob/master/common/pom.xml,

        <dependency>
          <groupId>joda-time</groupId>
          <artifactId>joda-time</artifactId>
          <version>2.2</version>
      </dependency>

And no exception is thrown,

$ ./run.sh  
<h1>Report from /home/old-meteo/imos/projects/docker_ncwms_s3/IMOS/ACORN/gridded_1h-avg-current-map_QC/ROT/2014/01/02/IMOS_ACORN_V_20140102T053000Z_ROT_FV01_1-hour-avg.nc</h1>
log4j:WARN No appenders could be found for logger (ucar.nc2.NetcdfFile).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.

I'm not sure if it's a joda library or edal-java issue, but I thought it was worth noting here. Cheers.

guygriffiths commented 7 years ago

Sounds like it could be a project configuration issue. edal-common should pull in joda-time v2.2, as required (and it looks like it works when you use that version) - where is version 2.8.1 coming from? I guess you are using another library which uses 2.8.1?

julian1 commented 7 years ago

In my project pom I only included edal-cdm, and not edal-common. Maven didn't complain and unzipping the shaded jar shows it wasn't pulled in indirectly/transitively. I'm only using the dataset class and extracting variable metadata, grid bounds - no graphics etc. Everything works as expected (ignoring the joda issue).

If edal-common is required, then I can certainly add it. Otherwise just expressing the joda-time version explicitly is also fine for me.

guygriffiths commented 7 years ago

That's very strange. edal-cdm depends on edal-common - if you depend on edal-cdm it should absolutely pull in edal-common as a dependency. Can you post a copy of your pom.xml on here?

julian1 commented 7 years ago

https://github.com/julian1-testing/reconciliation-example/blob/master/pom.xml

julian1 commented 7 years ago

There is a edal-common pom in the resultant jar, but no edal-common class files.

$ unzip -l target/s3-example-1.0-SNAPSHOT-shaded.jar | grep edal-common
        0  2016-10-27 08:34   META-INF/maven/uk.ac.rdg.resc/edal-common/
     2308  2016-10-27 08:34   META-INF/maven/uk.ac.rdg.resc/edal-common/pom.xml
      111  2016-10-27 08:34   META-INF/maven/uk.ac.rdg.resc/edal-common/pom.properties
guygriffiths commented 7 years ago

I think the issue is that aws-java-sdk-s3 is pulling in joda-time. If I specifically exclude joda-time from aws-java-sdk-s3, and remove the joda-time artifact, then I think it will pull it in as a dependency. I can't be sure, because it doesn't build on my machine (for some reason, the source compatibility level is set to 1.3 and it won't compile because you've used generics), but using mvn dependency:tree suggests that's the case.

So this is an issue with your project configuration, which can be solved in multiple ways. However, it's probably worth me updating joda-time to the latest version in EDAL anyway, which may fix things for you.

julian1 commented 7 years ago

So this is an issue with your project configuration, which can be solved in multiple ways.

Yes. This is easily worked around.

For the most part, I just thought it was odd, that verifyValueBounds(DateTimeField field, int value, int lowerBound, int upperBound) which going by the signature is a very simple range check started to throw. So my concern was - that it was flagging some other logic issue somewhere (possibly in joda time).

guygriffiths commented 7 years ago

Yeah, I tend to agree that it could be a warning flag. Hopefully the update to joda time 2.9.4 (just released) will fix any problems before they arise...