GEOWAVE-1723: "filesystem" datastore with pluggable data formatter#1724
GEOWAVE-1723: "filesystem" datastore with pluggable data formatter#1724rfecher merged 1 commit intolocationtech:masterfrom
Conversation
jdgarrett
left a comment
There was a problem hiding this comment.
Overall it looks good, to summarize all of my review notes:
- There are a lot of missing copyright headers for new files
- There are some lingering references to RocksDB
- I have some concerns about integration tests where the file system data store is excluded or intermittently fails
| import org.locationtech.geowave.core.store.api.DataStore; | ||
| import org.locationtech.geowave.datastore.filesystem.FileSystemStoreFactoryFamily; | ||
| import org.locationtech.geowave.datastore.filesystem.config.FileSystemOptions; | ||
| import org.locationtech.geowave.datastore.rocksdb.util.RocksDBClientCache; |
|
I addressed the concerns. See comments regarding ITs failing, but they always pass locally for me so it is isolated to travis and more challenging to diagnose. I am running 2 passes of the rest of the tests on travis, the last one with secondary indexing enabled on all the tests so overall its pretty solid. But yes, I'd really prefer all tests to pass all the time on travis-ci. I can poke around more in the future if I have a spare cycle and renyone is welcome to poke around. |
jdgarrett
left a comment
There was a problem hiding this comment.
There are some references to RocksDB in the following files, once those have been resolved it will be good to merge:
FileSystemDefaultConfigProvider.java
FileSystemOperations.java
FileSystemStoreTestEnvironment.java
Signed-off-by: Rich Fecher <rich.fecher@blacklynx.tech>
|
good catch - fixed those rocks references and squashed it back down to a single commit |
See issue #1723 for details.
Signed-off-by: Rich Fecher rich.fecher@blacklynx.tech