Make PaperTable public and update kryo libraries #194
Conversation
|
@ahmetturk thank you very much for resolving those issues! I'll take a look as soon as I can |
There was a problem hiding this comment.
Thanks again for making this long waiting fix! I was trying to apply it and realised that PR contains seems unnecessary changes which also broke unit tests. Please consider making an isolated PR which solve the critical issue and make another PRs if you wish to make other improvements. Thank you!
| @@ -68,8 +68,8 @@ afterEvaluate { | |||
| } | |||
|
|
|||
| dependencies { | |||
| api 'com.esotericsoftware:kryo:4.0.1' | |||
| api 'de.javakaffee:kryo-serializers:0.40' | |||
| api 'com.esotericsoftware:kryo:5.2.0' | |||
There was a problem hiding this comment.
How necessary is it to update dependencies to fix #188? If not, I'd keep it for separate PR to make changes isolated (e.g. for revert or understand the history of changes)
There was a problem hiding this comment.
There are major changes from Kyro 4.0.1 to Kyro 5.2.0, which would require a migration as stated below:
https://github.com/EsotericSoftware/kryo/releases/tag/kryo-parent-5.0.0-RC1
https://github.com/EsotericSoftware/kryo/wiki/Migration-to-v5
I suggest we only fix for #188 and co
| @@ -48,16 +49,13 @@ private Kryo getKryo() { | |||
| private final ThreadLocal<Kryo> mKryo = new ThreadLocal<Kryo>() { | |||
| @Override | |||
| protected Kryo initialValue() { | |||
| return createKryoInstance(false); | |||
| return createKryoInstance(); | |||
There was a problem hiding this comment.
Should compatibility mode be removed in order to provide a fix for #188? If not, I'd put it into separate PR since bunch of tests has to be fixed as well.
| @@ -1,6 +1,6 @@ | |||
| package io.paperdb; | |||
|
|
|||
| class PaperTable<T> { | |||
| public class PaperTable<T> { | |||
There was a problem hiding this comment.
I guess this line provides exact fix for #188?
There was a problem hiding this comment.
Hey @pilgr,
I don't think that this will fix #188. I've tested it and also commented on the ticket.
But maybe this comment can help to solve the issue.
And I still think that the kryo library and maybe other dependencies should be updated. Just guessing but maybe that will get rid of the buffer underflow issue, which still occurs on our app. I can create a separate ticket for it, if it is wanted .
Best regards!
=> error: pilgr#4 They said it was fixed in paperdb 2.7.2, but not really! fix: pilgr#194 (upgrade kryo) => why (kryo.setRegistrationRequired(false);) EsotericSoftware/kryo#398 (comment)
This pull request fixes the issues #188 #189 #190 #191 #192 #193