Skip to content

RYA-534 Upgrade to RDF4J 3.2 and RYA-496 Upgrading to Accumulo 1.9.3 #315

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

RYA-534 Upgrade to RDF4J 3.2 and RYA-496 Upgrading to Accumulo 1.9.3 #315

wants to merge 5 commits into from

Conversation

@brushworth
Copy link

@brushworth brushworth commented Jun 30, 2020 •
edited
Loading

Description

I've upgrade the RDF4J dependencies to the latest. I've upgraded other dependencies including Accumulo.

This PR needs to be merged before #316

Tests

Yes, tests have been updates where required by dependency changes.

Links

RYA-534
RYA-496

Checklist

  • Code Review
  • Squash Commits

People To Reivew

@adinancr
@jdasch
@amihalik

@brushworth brushworth marked this pull request as ready for review June 30, 2020 08:37
DLotts
DLotts requested changes Jul 7, 2020
View reviewed changes
@brushworth brushworth mentioned this pull request Jul 22, 2020
Open
2 tasks
@wollefitz
Copy link
Contributor

wollefitz commented Nov 10, 2020 •
edited
Loading

This PR has been open for a while already - is there any possibility that it could be merged soon (once the conflicts are resolved)? This is needed for making Rya compatible with Java 11 (see apache/accumulo#942 and https://issues.apache.org/jira/browse/RYA-471)

DLotts
DLotts requested changes Nov 11, 2020
View reviewed changes
Copy link

@DLotts DLotts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all this hard work. Note that even though I marked this review as "request changes", the requests are not fixes, but asking for more upgrades: latest accumulate and latest rdf4j. Several updates have come thru for both of them over the summer. I am happy taking this as-is since it still a big jump for our dependencies upgrades. :-) So just mark them "resolved" and we'll catch the increments later.

pom.xml Outdated

<accumulo.version>1.6.4</accumulo.version> <!-- Newest: 1.7.0 -->
<hadoop.version>2.5.0</hadoop.version> <!-- Newest: 2.7.1 -->
<accumulo.version>1.9.3</accumulo.version>
Copy link

@DLotts DLotts Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.9.3 was probably the latest when you did this. How difficult would be to go to Accumulo version to 1.10.0 -- their LTM release? I'm perfectly fine with accepting this as is, as you did lots of working getting this done. Perhaps you have already done this, so I thought I would bring it up.

Copy link
Author

@brushworth brushworth Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pom.xml Outdated
</modules>
<properties>
<org.eclipse.rdf4j.version>2.3.1</org.eclipse.rdf4j.version> <!-- Newest: 2.3.1 -->
<org.eclipse.rdf4j.version>3.2.2</org.eclipse.rdf4j.version>
Copy link

@DLotts DLotts Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, RDF4J is moving fast. It is now at 3.4.3. I believe that you used the latest version when you did this work. How hard would it be to use the latest (again)?

Copy link
Author

@brushworth brushworth Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, its already 3.4.4, working on it now.

Copy link
Author

@brushworth brushworth Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

extras/indexing/src/main/java/org/apache/rya/indexing/entity/model/Property.java
* A value that has been set for an {@link TypedEntity}.
*/
@Immutable
@Contract(threading = ThreadingBehavior.IMMUTABLE)
Copy link

@DLotts DLotts Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explainer: https://dev-aux.com/java/org-apache-http-annotation-threadsafe-class-not-found

...treams/api/src/test/java/org/apache/rya/streams/api/queries/InMemoryQueryRepositoryTest.java
final QueryRepository queries = new InMemoryQueryRepository( changeLog, SCHEDULE );
try {
queries.startAndWait();
queries.startAsync();
Copy link

@DLotts DLotts Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Did you have a source article for these changes?

Copy link
Author

@brushworth brushworth Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old method was removed by Guava, it was previously deprecated: https://guava.dev/releases/16.0/api/docs/com/google/common/util/concurrent/Service.html#startAndWait()

Copy link
Author

@brushworth brushworth Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't bother with awaitRunning() etc for test code because it didn't seem to need it, but I included it for all production code.

@brushworth brushworth force-pushed the RYA-534_rdf4j_squash_2 branch from a365376 to dc8896d Compare November 16, 2020 05:30
@brushworth
Copy link
Author

brushworth commented Nov 16, 2020

Changes made

@DLotts
Copy link

DLotts commented Nov 23, 2020

It looks like org.locationtech.spatial4j has been refactored. I renamed from com.spatial4j to org.locationtech.spatial4j but it seems there is more changes. Can you take a look?

 `package org.locationtech.spatial4j.core.context does not exist`

Here is more:

[ERROR] /home/vagrant/rya/extras/rya.pcj.fluo/rya.pcj.functions.geo/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/function/geosp
arql/SpatialSupportInitializer.java:[23,47] package org.locationtech.spatial4j.core.context does not exist
[ERROR] /home/vagrant/rya/extras/rya.pcj.fluo/rya.pcj.functions.geo/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/function/geosp
arql/SpatialSupportInitializer.java:[24,51] package org.locationtech.spatial4j.core.context.jts does not exist
[ERROR] /home/vagrant/rya/extras/rya.pcj.fluo/rya.pcj.functions.geo/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/function/geosp
arql/SpatialSupportInitializer.java:[25,45] package org.locationtech.spatial4j.core.shape does not exist
[ERROR] /home/vagrant/rya/extras/rya.pcj.fluo/rya.pcj.functions.geo/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/function/geosparql/SpatialSupportInitializer.java:[35,15] cannot find symbol
  symbol:   class SpatialContext
  location: class org.eclipse.rdf4j.query.algebra.evaluation.function.geosparql.SpatialSupportInitializer

@brushworth
Copy link
Author

brushworth commented Jan 8, 2021 •
edited
Loading

It looks like org.locationtech.spatial4j has been refactored. I renamed from com.spatial4j to org.locationtech.spatial4j but it seems there is more changes. Can you take a look?

This should all be fixed now.

Apologies if merging in the latest master branch changes was a bad idea but I noticed a bug which I discovered had already been fixed by yourself late last year, so I pulled it in. I'm not sure what Rya's merging vs rebasing etc policy might be.

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

Reviewers

@DLotts DLotts DLotts requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@brushworth @wollefitz @DLotts