Add forward compatibility to TaggedFieldSerializer in issue #352. #365

levyfan · 2015-11-10T13:03:43Z

Add forward compatibility to TaggedFieldSerializer in issue #352.

Add ignoreUnknownTags flag in Kryo configuration. If it is set, TaggedFieldSerializer will ignore unknown field tags in input stream. Otherwise a KryoException will be thrown.

magro · 2016-02-02T22:34:53Z

LGTM. @NathanSweet @romix What do you think?

psmarcos · 2016-02-03T15:29:03Z

+1

magro · 2016-02-20T00:12:47Z

@levyfan Can you adjust javadoc and the README to mention this improvement? I think then it's ok to merge. Thanks!

levyfan · 2016-03-14T06:16:50Z

Thanks @magro , please have a review.

fgaule · 2016-03-16T23:34:14Z

Great to have this feature soon :)

magro · 2016-03-17T07:19:28Z

I've thought about the way of configuration and think we should choose a different approach: https://groups.google.com/forum/#!topic/kryo-users/G16daCijbWo

So if we merge this now the api might be changed before the release, or we wait until this is clarified / changed.

Apart from this, there are things currently holding back a release: https://groups.google.com/forum/#!topic/kryo-users/Fg0yCqZts6E

psmarcos · 2016-03-23T15:43:55Z

@magro @levyfan
The serializer fails when you add a field that is not sorted alphabetically. Example:

class NewRoot {
  @TaggedFieldSerializer.Tag(0)
  field b;
  @TaggedFieldSerializer.Tag(1)
  field a;
}

class Root {
 @TaggedFieldSerializer.Tag(0)
 field b;
}

If the serializer writes a byte stream with type NewRoot then it can't be read correctly into an object of type Root.

However if alphabetical order is maintained:

class NewRoot {
  @TaggedFieldSerializer.Tag(0)
  field b;
  @TaggedFieldSerializer.Tag(1)
  field c;
}

class Root {
 @TaggedFieldSerializer.Tag(0)
 field b;
}

The stream can be read correctly into an object of type Root .

Here's a file with tests
TaggedFieldSerializerTest.java.zip

magro · 2016-03-25T23:59:29Z

@levyfan @psmarcos Is anybody of you going to fix this? Good catch btw!

levyfan · 2016-03-28T06:00:56Z

@magro @psmarcos fixed.
Now TaggedFieldSerializer will write from lowest tag to highest tag, for example, tag 0,1,2,...,N. If the reader do not have any information about tag i, then the stream from tag i will be ignored, i.e, tag i,i+1,...,N won't be read. (Because the reader do not know how many bytes tag i takes and what is the start point of tag i+1).

The `Kryo` class is more and more used for "global" configuration of serializers like `FieldSerializer` etc (here's another PR: EsotericSoftware#365). The semantics of such settings and especially for which serializer it's applied can often only be clarified via documentation. This change introduces a `FieldSerializerConfig` class that encapsulates configuration for `FieldSerializer`. It allows to configure settings for all `FieldSerializer` instances created via `Kryo.getFieldSerializerConfig`. `Kryo.asmEnabled` might be used by different serializers, but because until now it was only used by `FieldSerializer` this setting is also kept in `FieldSerializerConfig`. `Kryo.set/getAsmEnabled` is marked as deprecated with a hint to use `getFieldSerializerConfig` instead - this is done to stay binary compatible. `Kryo.setCopyTransient` is removed because it was introduced after the latest release 3.0.3 and therefore not yet a published/released. Now `Kryo.getFieldSerializerConfig.setCopyTransient` has to be used. If this is considered to be merged then EsotericSoftware#365 should also follow this approach.

The `Kryo` class is more and more used for "global" configuration of serializers like `FieldSerializer` etc (here's another PR: EsotericSoftware#365). The semantics of such settings and especially for which serializer it's applied can often only be clarified via documentation. This change introduces a `FieldSerializerConfig` class that encapsulates configuration for `FieldSerializer`. It allows to configure settings for all `FieldSerializer` instances created via `Kryo.getFieldSerializerConfig`. On each `FieldSerializer` the config can be changed/overridden, this is done directly on the serializer (instead e.g. requiring to invoke `fieldSerializer.getConfig.setSomeThing`) to stay binary compatible and because forcing to use the config here does not seem to be of much value. The `FieldSerializer` internally uses the config object so that any further settings are directly added to `FieldSerializerConfig` and there's only a single place to keep config settings. `Kryo.asmEnabled` might be used by different serializers, but because until now it was only used by `FieldSerializer` this setting is also kept in `FieldSerializerConfig`. `Kryo.set/getAsmEnabled` is marked as deprecated with a hint to use `getFieldSerializerConfig` instead - this is done to stay binary compatible. `Kryo.setCopyTransient` is removed because it was introduced after the latest release 3.0.3 and therefore not yet a published/released. Now `Kryo.getFieldSerializerConfig.setCopyTransient` has to be used. If this is considered to be merged then EsotericSoftware#365 should also follow this approach.

levyfan · 2016-04-06T02:12:11Z

@magro please have a review

magro · 2016-04-06T06:57:13Z

Because the ignoreUnknownTags setting doesn't make sense for FieldSerializer I'd prefer to have a TaggedFieldSerializerConfig (extending FieldSerializerConfig).

magro · 2016-04-06T07:00:28Z

src/com/esotericsoftware/kryo/serializers/FieldSerializer.java

+	 * </p>
+	 *
+	 * @param ignoreUnknownTags if true, unknown field tags will be ignored. Otherwise KryoException will be thrown */
+	public void setIgnoreUnknownTags (boolean ignoreUnknownTags) {


This should go into TaggedFieldSerializer.

levyfan · 2016-04-10T07:37:48Z

Thanks @magro , please have a review

magro · 2016-04-10T08:07:25Z

src/com/esotericsoftware/kryo/serializers/FieldSerializer.java

@@ -70,7 +70,7 @@
 	/** type variables declared for this type */
 	final TypeVariable[] typeParameters;
 	final Class componentType;
-	private final FieldSerializerConfig config;
+	final FieldSerializerConfig config;


Semantically this should be protected probably, sorry that I haven't seen this last time.

magro · 2016-04-10T08:08:47Z

LGTM, only the one protected seems to be left. Can you squash commits to a single one? Then I'll merge. Thanks!

levyfan · 2016-04-10T18:26:43Z

Hi @magro , please have a review

magro · 2016-04-10T20:24:36Z

Great, thanks a lot!

magro mentioned this pull request Mar 28, 2016

Introduce FieldSerializerConfig to encapsulate config in Kryo #414

Merged

levyfan closed this Apr 6, 2016

levyfan reopened this Apr 6, 2016

magro reviewed Apr 6, 2016
View reviewed changes

magro reviewed Apr 10, 2016
View reviewed changes

Add forward compatibility to TaggedFieldSerializer in issue #352.

93bff2d

magro merged commit 6483e86 into EsotericSoftware:master Apr 10, 2016

magro mentioned this pull request Apr 10, 2016

Add forward compatibility to TaggedFieldSerializer #352

Closed

levyfan deleted the issue#352 branch April 11, 2016 02:31

cypherdare mentioned this pull request Jul 9, 2016

TaggedFieldSerializer not actually forward compatible #442

Closed

Add forward compatibility to TaggedFieldSerializer in issue #352. #365

Add forward compatibility to TaggedFieldSerializer in issue #352. #365

Uh oh!

Conversation

levyfan commented Nov 10, 2015

Uh oh!

magro commented Feb 2, 2016

Uh oh!

psmarcos commented Feb 3, 2016

Uh oh!

magro commented Feb 20, 2016

Uh oh!

levyfan commented Mar 14, 2016

Uh oh!

fgaule commented Mar 16, 2016

Uh oh!

magro commented Mar 17, 2016

Uh oh!

psmarcos commented Mar 23, 2016

Uh oh!

magro commented Mar 25, 2016

Uh oh!

levyfan commented Mar 28, 2016

Uh oh!

levyfan commented Apr 6, 2016

Uh oh!

magro commented Apr 6, 2016

Uh oh!

magro Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

levyfan commented Apr 10, 2016

Uh oh!

magro Apr 10, 2016

Choose a reason for hiding this comment

Uh oh!

magro commented Apr 10, 2016

Uh oh!

levyfan commented Apr 10, 2016

Uh oh!

magro commented Apr 10, 2016

Uh oh!

Uh oh!