Skip to content

Conversation

@jamesmcclain
Copy link
Member

@jamesmcclain jamesmcclain commented Jun 7, 2017

James McClain added 2 commits June 7, 2017 18:31
Taken from GeoWave commit 73ae05af4358d5c8741963eb692a9189ebf616b9.

Signed-off-by: James McClain <jmcclain@azavea.com>
@jamesmcclain jamesmcclain force-pushed the feature/geowave branch 2 times, most recently from c42e64e to fe62dfb Compare June 8, 2017 16:20
@jamesmcclain jamesmcclain changed the title [WIP] Import GeoWave Indexing Code Import GeoWave Indexing Code Jun 13, 2017
James McClain added 3 commits June 13, 2017 11:28
@jnh5y
Copy link
Contributor

jnh5y commented Jun 13, 2017

@jamesmcclain Thanks. There are a few things we'd need to work through in terms of accepting a contribution like this.

First, I believe the contribution would need to go through the Eclipse legal review. To my knowledge, GeoWave's code base has not been completely reviewed. Sizable contributions need to be checked off and reviewed for IP and provenance reasons.

Second, I believe we need to get an ok from the companies whose copyright are involved. Even if the code is open-source, contributing major parts of one codebase to another requires working through some details.

Third, we should identify the new dependencies that this contribution adds to SFCurve. Those dependencies need to reviewed and approved by Eclipse before we can accept this PR.

Fourth, I'd like to see the package names changed to be consistent.

Those are just some initial issues to address. Since the contribution is sizable, a more complete review will take some time.

@jamesmcclain
Copy link
Member Author

Okay, sounds good.

@lossyrob and @echeipesh may wish to chime in, as well.

@lossyrob
Copy link
Member

@rfecher and I had talked about this work before (a while ago, at FedGeoDay). His comments here would be appreciated.

Makes sense about the CQs and legal review. I commented on the mailing list:

According to this: https://github.com/locationtech/geowave/blob/master/core/index/pom.xml, the only new dependencies would be the findbugs (which is a bad dep according to eclipse) and json-lib (http://json-lib.sourceforge.net/, which seems based on json.org work - and we've had problems with that dependency before).

Problems with these dependencies would be problems with GeoWave, so good to catch these early.

Package name consistency would be good. I do think that a main goal of this first step would be to get this code into the SFCurve repository, with an eye towards merging/combining this work and the current SFCurve work into a single API. A first desire for us is to utilize the periodicity feature in GeoWave indexing. Part of this work is also knocking out the outstanding task of having GeoTrellis depend on SFCurve.

@rfecher
Copy link

rfecher commented Jun 13, 2017

I'm all for figuring out how all projects benefit by leveraging each other's code here. I think it makes sense to have a discussion over what it should look like, but if @jamesmcclain has the time right now to take a stab at it, I wouldn't discourage that either. Of course as far as pulling anything in, that's up to the project leads and the CQ process.

The json dependency was because one of our contributors thought it looked nicer to print index config/info in a json format through our CLI commands. Its pretty unessential, or could be replaced easily by any other comparable json library that is eclipse-approved. Findbugs is obviously unessential - we could check what the "depends on" definition is with eclipse, but it doesn't seem like static code analysis tooling would fit that definition to me anyways. Often we build with -Dfindbugs.skip flag for speed which ignores it completely, but we do like our CI to flag potential code quality issues for us.

For copyright, I personally like to see "Copyright contributors to the Eclipse Foundation" for source headers and then we can put each individual company that contributes in a separate notice file. I find it more unifying, particularly for a project like this with a good mix of contributors, than labeling each individual source file with a company. Again, of course that call is up to the leads...

I'm off today and another week (back in on June 21)...just saying I may not be responsive, but will try to stay caught up on this issue.

@jamesmcclain
Copy link
Member Author

I have removed FindBugs and it looks as though there is an approved CQ for the json library (thank you @lossyrob ).

This does not address all comments, but should provide partial satisfaction.

@jamesmcclain jamesmcclain force-pushed the feature/geowave branch 2 times, most recently from 492881c to 4ad3d96 Compare June 14, 2017 17:24
@jnh5y
Copy link
Contributor

jnh5y commented Jun 14, 2017

@jamesmcclain thanks! @echeipesh @lossyrob can you all handle a CQ for this contributed code and piggyback CQs for any dependencies that SFCurve will need for this PR?

@lossyrob
Copy link
Member

lossyrob commented Jul 13, 2017

@jnh5y I wrote the piggyback CQ for the json library. CQ 13868
It seems like GeoWave is approved for parallel check-in (see CQ 13537), and the code lives in the locationtech github organization, so I don't believe we'll need to write a contribution CQ for this as it's already locationtech code the IP team is aware of.

@jnh5y
Copy link
Contributor

jnh5y commented Jul 13, 2017

Let's ask emo/Wayne, etc about it. Possibly we can get Sharon to sort through this portion of the code sooner, etc.

What's the status on updating the package names, etc? That should be handled during this initial contribution.

@pomadchin
Copy link
Member

pomadchin commented Jul 14, 2017

@lossyrob
Copy link
Member

Superseded by #20

@lossyrob lossyrob closed this Sep 22, 2017
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.

5 participants