Update to Java 17 and Kryo 5 #3994

hannesj · 2022-03-16T14:20:56Z

Summary

Update to Java 17. I needed to update a couple of libraries, in order not to have compering files of some of the dependencies.

Added UnmodifiableCollectionsSerializer from external source Compatibility with JDK 17 magro/kryo-serializers#131 (comment)
Added JavaImmutableMapSerializer, as the Kryo built-in serializer sometimes returned the wrong type.

Changelog

The changelog file
is generated from the pull-request title, make sure the title describe the feature or issue fixed.
To exclude the PR from the changelog add [changelog skip] in the title.

leonardehrenfried · 2022-03-16T16:18:05Z

src/main/java/org/opentripplanner/kryo/UnmodifiableCollectionsSerializer.java

+ *
+ * @author <a href="mailto:[email protected]">Martin Grotzke</a>
+ */
+public class UnmodifiableCollectionsSerializer extends Serializer<Object> {


Can these be remove once we move to Kryo 5?

It can be removed when issue magro/kryo-serializers#131 is fixed. The PR contains an update to Kryo 5

src/main/java/org/opentripplanner/graph_builder/module/geometry/GeometryAndBlockProcessor.java

t2gran · 2022-03-16T16:43:38Z

This looks great. I only had one question, se above.

leonardehrenfried · 2022-03-16T16:46:32Z

src/main/java/org/opentripplanner/routing/graph/kryosupport/KryoBuilder.java

@@ -52,44 +48,28 @@ public static Kryo create() {
        kryo.register(TIntIntHashMap.class, new TIntIntHashMapSerializer());

        // Add support for the package local java.util.ImmutableCollections.
-        // Not supported in the current com.conveyal:kryo-tools:1.3.0.
+        // Not supported properly in the current com.conveyal:kryo-tools:1.4.0.
        // This provide support for List.of, Set.of, Map.of and Collectors.toUnmodifiable(Set|List|Map)
        kryo.register(List.of().getClass(), new JavaImmutableListSerializer());


Doesn't Kryo 5 have built-in support for List.of()? https://github.com/EsotericSoftware/kryo/blob/master/src/com/esotericsoftware/kryo/serializers/ImmutableCollectionsSerializers.java

Or are they broken in the same was as the map one?

Yes, they will return a wrong type when serializing an empty Set/List/Map, thus failing the tests which compare the loaded graph. I'll create an issue in kryo about that

https://github.com/EsotericSoftware/kryo/blob/master/src/com/esotericsoftware/kryo/serializers/ImmutableCollectionsSerializers.java#L51-L53

and

https://github.com/EsotericSoftware/kryo/blob/master/src/com/esotericsoftware/kryo/serializers/CollectionSerializer.java#L216-L219

Ok, thanks for the explanation. Can you cc me when you create the ticket?

abyrd · 2022-03-17T03:24:31Z

Is this intended to be merged into dev-2.x before or after the release? I ask because there's already a 2.1-rc branch and this PR seems like a major change to introduce immediately before a release. Actually I may be misinterpreting the branching strategy because it's different than what we've used in the past. It looks like development is continuing on dev-2.x and changes intended for the release are being cherry-picked over to 2.1-rc.

hannesj · 2022-03-17T06:53:31Z

This is intended for 2.2, so after the release has been cut

t2gran · 2022-03-17T09:29:03Z

@abyrd I already updated the version number in dev-2.x to 2.2.0-SNAPSHOT. There is a stream of PRs constantly beeing merged and I did not want everyone to change their habit of merging into dev-2.x, so we branched of 2.1-rc at the point we thought was the best place, and then we have cherry-picked a few PRs/commits from dev-2.x after that. I plan to merge the master branch back into dev-2.x when the release is done, so we get all change (also the one done locally in 2.1-rc) in the dev-2.1. It is a bit messy - for me - but cleaner for every one else.

theigl · 2023-04-29T07:46:18Z

Hi @hannesj. I'm the maintainer of Kryo and just stumbled across this issue. Are you sure that the immutable collections serializers really return the wrong type at times? I just wrote a a test-case that asserts on the result type of the collection and it passes for all sizes.

leonardehrenfried · 2023-04-29T08:59:56Z

Hi @theigl, Hannes isn't contributing a lot anymore.

However, I can try removing our custom serializers and run the test suite again. Maybe these things have been fixed in the meantime.

Thanks for chiming in!

theigl · 2023-04-29T09:10:45Z

@leonardehrenfried: Thanks, that would be great. I'd really like to know if there is some hidden bug in those serializers that I'm currently not aware of.

leonardehrenfried · 2023-04-29T09:36:40Z

So I just tried it by removing our custom serializers and using the built-in Kyro ones: leonardehrenfried@591a54d

(Is this setup correct?)

Unfortunately, after this change our test reports that we expect at SetN but we receive a HashSet instead.

Classes are not the same: SetN vs HashSet
Comparison stack (outermost first):
    class org.opentripplanner.routing.graph.Graph
    private final java.util.Map org.opentripplanner.routing.graph.Graph.vertices
    class java.util.concurrent.ConcurrentHashMap
    EN:NSR:Quay:122003
    class org.opentripplanner.street.model.vertex.TransitStopVertex
    private final org.opentripplanner.transit.model.site.RegularStop org.opentripplanner.street.model.vertex.TransitStopVertex.stop
    class org.opentripplanner.transit.model.site.RegularStop
    private final java.util.Set org.opentripplanner.transit.model.site.RegularStop.fareZones

Something is amiss here. It could very well be that our setup is wrong.

leonardehrenfried · 2023-04-29T10:18:10Z

I wrote some more tests and I can confirm that with plain Kryo the type is correctly deserialized to SetN not HashMap. Presumably something is wrong with our setup.

Can you spot an error in our KryoBuilder? https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/main/java/org/opentripplanner/routing/graph/kryosupport/KryoBuilder.java

theigl · 2023-04-29T10:38:54Z

Everything looks alright in your builder. I'll try to get to the bottom of this next week. Your branch should be enough for me to write a reproducer.

Thanks!

leonardehrenfried · 2023-04-29T11:02:51Z

Great. If you want to run the test yourself it's called GraphSerializationTest.

hannesj added 2 commits March 16, 2022 15:10

Remove unused field from graph

396971e

Update to Java 17

502b6a2

hannesj added the Technical Debt label Mar 16, 2022

hannesj added this to the 2.2 milestone Mar 16, 2022

hannesj requested a review from a team as a code owner March 16, 2022 14:20

hannesj added 4 commits March 16, 2022 16:31

Remove old warning

ffbe67e

Revert change in snapshot

632f80d

Remove deprecated flag

e53a333

Update dependencies

d73d9eb

leonardehrenfried reviewed Mar 16, 2022

View reviewed changes

hannesj changed the title ~~Update to Java 17~~ Update to Java 17 and Kryo 5 Mar 16, 2022

t2gran reviewed Mar 16, 2022

View reviewed changes

src/main/java/org/opentripplanner/graph_builder/module/geometry/GeometryAndBlockProcessor.java Show resolved Hide resolved

leonardehrenfried reviewed Mar 16, 2022

View reviewed changes

t2gran previously approved these changes Mar 16, 2022

View reviewed changes

Bump serialization version

640cf92

hannesj dismissed t2gran’s stale review via 640cf92 March 16, 2022 21:00

hannesj added 2 commits March 17, 2022 16:43

Bump serialization version

d11eea9

Merge remote-tracking branch 'origin/dev-2.x' into otp2_java17

97489ee

t2gran approved these changes Mar 17, 2022

View reviewed changes

leonardehrenfried approved these changes Mar 17, 2022

View reviewed changes

hannesj merged commit a8287d3 into opentripplanner:dev-2.x Mar 17, 2022

hannesj deleted the otp2_java17 branch March 17, 2022 15:59

t2gran pushed a commit that referenced this pull request Mar 17, 2022

Add changelog entry for #3994 [ci skip]

f920682

t2gran added Skip Changelog and removed Skip Changelog labels Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Java 17 and Kryo 5 #3994

Update to Java 17 and Kryo 5 #3994

hannesj commented Mar 16, 2022

leonardehrenfried Mar 16, 2022

hannesj Mar 16, 2022 •

edited

Loading

t2gran commented Mar 16, 2022

leonardehrenfried Mar 16, 2022

hannesj Mar 16, 2022

hannesj Mar 16, 2022

leonardehrenfried Mar 16, 2022

abyrd commented Mar 17, 2022 •

edited

Loading

hannesj commented Mar 17, 2022 •

edited

Loading

t2gran commented Mar 17, 2022 •

edited

Loading

theigl commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

theigl commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

theigl commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

Update to Java 17 and Kryo 5 #3994

Update to Java 17 and Kryo 5 #3994

Conversation

hannesj commented Mar 16, 2022

Summary

Changelog

leonardehrenfried Mar 16, 2022

Choose a reason for hiding this comment

hannesj Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

t2gran commented Mar 16, 2022

leonardehrenfried Mar 16, 2022

Choose a reason for hiding this comment

hannesj Mar 16, 2022

Choose a reason for hiding this comment

hannesj Mar 16, 2022

Choose a reason for hiding this comment

leonardehrenfried Mar 16, 2022

Choose a reason for hiding this comment

abyrd commented Mar 17, 2022 • edited Loading

hannesj commented Mar 17, 2022 • edited Loading

t2gran commented Mar 17, 2022 • edited Loading

theigl commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

theigl commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

theigl commented Apr 29, 2023

leonardehrenfried commented Apr 29, 2023

hannesj Mar 16, 2022 •

edited

Loading

abyrd commented Mar 17, 2022 •

edited

Loading

hannesj commented Mar 17, 2022 •

edited

Loading

t2gran commented Mar 17, 2022 •

edited

Loading