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

Create GeoWave Subproject #1542

Merged
merged 38 commits into from Oct 5, 2016

Conversation

Projects
None yet
6 participants
@jamesmcclain
Member

jamesmcclain commented Jun 11, 2016

These changes add a GeoWave subproject, which includes a GeowaveAttributeStore, GeowaveLayerReader, and GeowaveLayerWriter.

  • Missing metadata exception
  • Reduction or elimination of use of asInstanceOf
  • Fix geotools Tests
  • Find work-arounds for bugs
  • Unit Tests
  • GridCoverage2D Kryo Serializer
  • Address Feedback
  • Hillshade Demo

Tests can be run locally and they pass, but there appears to be insufficient memory to bring up GeoWave on Travis:

OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x0000000768680000, 51904512, 0) failed; error='Cannot allocate memory' (errno=12)
#
# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (malloc) failed to allocate 51904512 bytes for committing reserved memory.
# An error report file with more information is saved as:
# /home/travis/build/geotrellis/geotrellis/hs_err_pid4764.log
+exit 1
The command ".travis/build-and-test.sh" exited with 1.
cache.2
store build cache
0.00s
2.98snothing changed, not updating cache
Done. Your build exited with 1.

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch 4 times, most recently from 712b4ac to 70520b1 Jun 11, 2016

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch 2 times, most recently from 21249d4 to b8501be Jun 18, 2016

if (implicitly[ClassTag[K]].toString != "geotrellis.spark.SpatialKey")
throw new Exception("Unsupported Key Type")
/* XXX numPartitions, and filterIndexOnly are currently ignored. */

This comment has been minimized.

@lossyrob

lossyrob Jun 20, 2016

Member

In GeoWave, is it always 1 index == 1 tile? If so, then you're not really ignoring filter index only, it's just by default doing either true or false behavior. filterIndexOnly is for when the index has more than one tile stored in the values, so that when we are updating tile entries in a backend we can update the tile information while not losing the surrounding tile info.

This comment has been minimized.

@jamesmcclain

jamesmcclain Jun 20, 2016

Member

If I understand your usage of the word "index" above, then I believe there is one index (one address) for one tile and one tile for one address. Thanks for the clarification on flterIndexOnly.

println(s"KEYBOUNDS=$kbs")
val geom = kbs
.map({ kb => fn(kb.asInstanceOf[KeyBounds[SpatialKey]]) })
.foldLeft(∅)({ (l, r) => l.union(r) })

This comment has been minimized.

@lossyrob

lossyrob Jun 20, 2016

Member

Where is ∅ defined?

This comment has been minimized.

@jamesmcclain
val Extent(lat, lng, _, _) = GridCoverage2DConverters.getExtent(gc)
val key = SpatialKey(
(lng / ranges(1)).toInt - minRow, // sic
maxCol - (lat / ranges(0)).toInt // sic

This comment has been minimized.

@lossyrob

This comment has been minimized.

@jamesmcclain

jamesmcclain Jun 20, 2016

Member

It is typically used when quoting to indicate that no transcription error has not been made, even though the quoted material might appear to be incorrect. I am using it in a slightly more broad way here: to indicate that (I believe) no error has been made even though it might look incorrect at first blush.

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch 3 times, most recently from c3f6159 to 3164a3a Jul 1, 2016

@jamesmcclain jamesmcclain changed the title from [WiP] Create GeoWave Subproject to Create GeoWave Subproject Jul 6, 2016

@jamesmcclain jamesmcclain changed the title from Create GeoWave Subproject to [WiP] Create GeoWave Subproject Jul 7, 2016

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch from 3164a3a to e665d9a Jul 8, 2016

@jamesmcclain jamesmcclain changed the title from [WiP] Create GeoWave Subproject to Create GeoWave Subproject Jul 8, 2016

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch from e665d9a to d81c543 Jul 8, 2016

@jamesmcclain jamesmcclain changed the title from Create GeoWave Subproject to [WiP] Create GeoWave Subproject Jul 14, 2016

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch from d81c543 to 3b03c22 Aug 28, 2016

@jamesmcclain jamesmcclain changed the title from [WiP] Create GeoWave Subproject to Create GeoWave Subproject Aug 28, 2016

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch 4 times, most recently from 95f02fe to 27c0035 Aug 28, 2016

.map { kv =>
val List(name, zoomStr) = kv.getKey.getRow.toString.split(SEP).toList
LayerId(name, zoomStr.toInt)
}
.toList
.distinct
scanner.close ; retval

This comment has been minimized.

@lossyrob

lossyrob Aug 30, 2016

Member

A more safe way to write this would be

  val scanner = connector.createScanner(attributeTable, new Authorizations())
  try {
   scanner.iterator 
        .map { kv =>
          val List(name, zoomStr) = kv.getKey.getRow.toString.split(SEP).toList
          LayerId(name, zoomStr.toInt)
        }
        .toList
        .distinct
  } finally {
    scanner.close()
  }

The rule generally followed with parans or no parans on a method that takes no arguments is that, if the method has side effects, include the parans so that it would never be mistaken for a val

This comment has been minimized.

@pomadchin

pomadchin Aug 31, 2016

Member

@lossyrob I can takesimilar changes into account for hbase backend too

This comment has been minimized.

@jamesmcclain

jamesmcclain Sep 14, 2016

Member

Well taken, it looks like Eugene addressed this one.

scanner.iterator.map(_.getKey.getColumnFamily.toString).toVector
val retval = scanner.iterator.map(_.getKey.getColumnFamily.toString).toVector
scanner.close ; retval

This comment has been minimized.

@lossyrob

lossyrob Aug 30, 2016

Member

Same finally comment as above

This comment has been minimized.

@jamesmcclain

jamesmcclain Sep 14, 2016

Member

It looks like Eugene addressed this one.

GridCoverage2DConverters.getCrs(self) match {
case Some(crs) =>
ProjectedRaster(toRaster(bandIndex), crs)
case None =>
// Default LatLng
ProjectedRaster(toRaster(bandIndex), LatLng)
}
}

This comment has been minimized.

@lossyrob

lossyrob Aug 30, 2016

Member

Why add the unnecessary braces?

This comment has been minimized.

@jamesmcclain

jamesmcclain Sep 14, 2016

Member

It looks like Eugene addressed this one.

def getGeotoolsCRS(crs: CRS): CoordinateReferenceSystem = {
crs.epsgCode match {
case Some(code) =>
authorityFactory.createCoordinateReferenceSystem(s"EPSG:${code}")

This comment has been minimized.

@lossyrob

lossyrob Aug 30, 2016

Member

Why the GeoToolsCRS => GeoToolsCRS.getAuthorityFactory(true).createCoordinateReferenceSystem(s"EPSG:${code}") change?

This comment has been minimized.

@echeipesh

echeipesh Sep 6, 2016

Contributor

Shouldn't be a reason for this, checking the GeoTools code the factories are already cached in a java static field.

This comment has been minimized.

@jamesmcclain

jamesmcclain Sep 14, 2016

Member

The motivation is to make sure that the coordinate order is well-specified. The authorityFactory variable is not recreated, so that seems okay. Since the value of the variable code is unknown, this seems to me to be required.

val list = Iterator.iterate(adapters)({ _ => adapters })
.takeWhile({ _ => adapters.hasNext })
.map(_.next.asInstanceOf[RasterDataAdapter])
.toArray

This comment has been minimized.

@lossyrob

lossyrob Aug 30, 2016

Member

This is a confusing use of Iterator.iterate. Why not just

import scala.collection.JavaConverters._

adapters.toScala
  .map({ case rda: RasterDataAdapter => rda })
  .toArray

?

This comment has been minimized.

@echeipesh

echeipesh Sep 6, 2016

Contributor

Pointing out new dependency in managed it is: https://github.com/jsuereth/scala-arm
Looks useful, but this is the first use of it. @lossyrob what do you think ?

This comment has been minimized.

@echeipesh

echeipesh Sep 6, 2016

Contributor

Refactored looks like:

  def adapters(bao: BasicAccumuloOperations): Array[RasterDataAdapter] =
    managed(new AccumuloAdapterStore(bao).getAdapters)
      .acquireAndGet { _.asScala.collect { case x: RasterDataAdapter => x } }
      .toArray

.getAdapters returns a ClosableIterator

This comment has been minimized.

@jamesmcclain

jamesmcclain Sep 14, 2016

Member

Eugene's PR included this change, but I had to back it out because it caused test failures:

[info] - should read an existing layer *** FAILED ***
[info]   java.lang.RuntimeException: The BatchScanner was unexpectedly closed while this Iterator was still in use. Ensure proper handling of the BatchScanner.
[info]   at org.apache.accumulo.core.client.impl.TabletServerBatchReaderIterator.hasNext(TabletServerBatchReaderIterator.java:187)
[info]   at mil.nga.giat.geowave.datastore.accumulo.metadata.AbstractAccumuloPersistence$NativeIteratorWrapper.hasNext(AbstractAccumuloPersistence.java:473)
[info]   at mil.nga.giat.geowave.core.store.CloseableIteratorWrapper.hasNext(CloseableIteratorWrapper.java:46)
[info]   at scala.collection.convert.Wrappers$JIteratorWrapper.hasNext(Wrappers.scala:41)
[info]   at scala.collection.Iterator$$anon$1.hasNext(Iterator.scala:847)
[info]   at scala.collection.Iterator$$anon$15.skip(Iterator.scala:451)
[info]   at scala.collection.Iterator$$anon$15.hasNext(Iterator.scala:452)
[info]   at scala.collection.Iterator$class.foreach(Iterator.scala:727)
[info]   at scala.collection.AbstractIterator.foreach(Iterator.scala:1157)
[info]   at scala.collection.generic.Growable$class.$plus$plus$eq(Growable.scala:48)
[info]   ...
return list
}
Array.empty[RasterDataAdapter]
}

This comment has been minimized.

@lossyrob

lossyrob Aug 30, 2016

Member

I would lose the return, as it's not idiomatic to return as a control struct.

A possible rewrite of this function:

def adapters(bao: BasicAccumuloOperations): Array[RasterDataAdapter] =
  for (adapters <- managed(new AccumuloAdapterStore(bao).getAdapters)) {
    adapters.toScala
      .map({ case rda: RasterDataAdapter => rda })
      .toArray
  }

@jamesmcclain jamesmcclain force-pushed the jamesmcclain:feature/jwm/GeoWave branch from 6543b16 to f27ea0f Oct 4, 2016

@lossyrob

This comment has been minimized.

Member

lossyrob commented Oct 5, 2016

Merging!

@lossyrob lossyrob merged commit 644c32b into locationtech:master Oct 5, 2016

1 check passed

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

This comment has been minimized.

Member

jamesmcclain commented Oct 5, 2016

😄

@jamesmcclain jamesmcclain deleted the jamesmcclain:feature/jwm/GeoWave branch Oct 5, 2016

@jamesmcclain jamesmcclain restored the jamesmcclain:feature/jwm/GeoWave branch Oct 7, 2016

@jamesmcclain jamesmcclain deleted the jamesmcclain:feature/jwm/GeoWave branch Oct 14, 2016

@lossyrob lossyrob added this to the 1.0 milestone Oct 18, 2016

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