msgpack-java icon indicating copy to clipboard operation
msgpack-java copied to clipboard

[0.7] backward compatibility with previous serialization api (0.6)

Open marenzo opened this issue 10 years ago • 24 comments

I'm trying to upgrade from 0.6 to 0.7, and from what I saw the protocol itself is backward compatible. We're having a lot of services out there which are using msgpack to communicate between them (RPC), and for doing this upgrade I must that the previous client (0.6) will know to deserialize 0.7 output, and vice-versa.

For now I'm having this two issues:

  • Having the following code:
if (len < (1 << 5)) {
    writeByte((byte) (FIXSTR_PREFIX | len));
}
else if (len < (1 << 8)) {
    writeByteAndByte(STR8, (byte) len);
}
else if (len < (1 << 16)) {
    writeByteAndShort(STR16, (short) len);
}
else {
    writeByteAndInt(STR32, len);
}

STR8 is not something that the previous client knows how to handle, and the fix I did locally was just commenting out else if (len < (1 << 8)). This thing should be probably configurable, if you want to provide easier migration path.

  • With the previous client, we serialized the values as Object array, that contained the fields the RPC needs to pass (may be primitives, String, Pojo's, and so on). The way we did serializing with the previous client was:
final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
final Packer packer = msgPack.createPacker(outputStream);

final Object[] params;
if (requestParams == null) {
  params = new Object[0];
} else {
  params = requestParams;
}

packer.writeArrayBegin(params.length);
for (final Object param : params) {
  packer.write(param);
}
packer.writeArrayEnd();

return outputStream.toByteArray();

now, with the new jackson databind I'm doing:

final ObjectMapper objectMapper = new ObjectMapper(new MessagePackFactory());
final Object[] params = new Object[]{ new MyPojo() }; // for example, may contain any other supported type / pojo
objectMapper.writeValueAsBytes(params);

and the byte array produced is not the same as the previous client, and it also contain all the fields of the pojo(?), which seems to miss one of the points of msgpack. but if I'm also doing: objectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);, the byte array not contains anymore all the field names, but still the produced byte array is not the same as with the previous client, which makes issues when trying to do de-serialize.

My questions are: is 0.7 should be backward compatible with 0.6 (in terms of consuming / producing same byte array)? why does MessagePackFactory (of the databind package) produces also the field names in the byte array, and the feature of disabling it is not documented anywhere? and last one is why does the produced output when serializing object array is different between 0.6 and 0.7 using the the specified API's?

Some relevant links:

our implementation with the previous client serializing: https://github.com/outbrain/ob1k/blob/ob1k-0.152/ob1k-core/src/main/java/com/outbrain/ob1k/common/marshalling/MessagePackRequestMarshaller.java#L213 deserializing: https://github.com/outbrain/ob1k/blob/ob1k-0.152/ob1k-core/src/main/java/com/outbrain/ob1k/common/marshalling/MessagePackRequestMarshaller.java#L166

with the new client (0.7): serializing: https://github.com/outbrain/ob1k/blob/msgpack_upgrade/ob1k-core/src/main/java/com/outbrain/ob1k/common/marshalling/MessagePackRequestMarshaller.java#L19 https://github.com/outbrain/ob1k/blob/msgpack_upgrade/ob1k-http/src/main/java/com/outbrain/ob1k/http/marshalling/JacksonMarshallingStrategy.java#L60 deserializing: https://github.com/outbrain/ob1k/blob/msgpack_upgrade/ob1k-core/src/main/java/com/outbrain/ob1k/common/marshalling/JacksonRequestMarshaller.java#L210

marenzo avatar Dec 30 '15 13:12 marenzo

@marenzo As for STR8, recent msgpack-java 0.6 seems to support it. https://github.com/msgpack/msgpack-java/blob/msgpack-0.6.12/src/main/java/org/msgpack/unpacker/MessagePackUnpacker.java#L211 Could you try 0.6.12?

    ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
    MessagePacker messagePacker = org.msgpack.core.MessagePack.newDefaultPacker(outputStream);
    byte[] bs = "0000111122223333444455556666777788889999".getBytes();
    messagePacker.packRawStringHeader(bs.length);
    messagePacker.writePayload(bs);
    messagePacker.close();

    Unpacker unpacker = new MessagePack().createUnpacker(new ByteArrayInputStream(outputStream.toByteArray()));
    System.out.println(unpacker.readString()); // works fine

komamitsu avatar Dec 30 '15 16:12 komamitsu

@komamitsu I can't, because of the reason that most of the deployed services are using 0.6.7, and I can't do one big rollout to all the services in production. I'll provide soon pull request for making str8 format configurable in 0.7. My mission is to have both 0.6 and 0.7 in production to work seamlessly

marenzo avatar Dec 30 '15 16:12 marenzo

RE serializing POJO, 0.6 serializes only values as an array w/o field names depending on the order that getDeclaredFields returns and it's a naive way and sometimes caused problems. So 0.7's jackson-dataformat-msgpack serializes POJO as a map (object in JSON) on jackson-databind framework and it's robust since the serialized data includes each key and value. As a result, the serialization of POJO between 0.6 and 0.7 is unfortunately incompatible.

komamitsu avatar Dec 31 '15 14:12 komamitsu

I understand your situation, but 0.6.7 is outdated. 0.6.12 includes bug fixes and improvements and I should recommend you to use 0.6.12 instead of 0.6.7. Also, making MessagePack 0.7 disable STR8 format in serialization doesn't sound an usual use case.

komamitsu avatar Jan 01 '16 06:01 komamitsu

The main issues here that msgpack-java doesn't provides documentation enough. Biggest thing that is missing for me is migration path - what does it mean upgrading from 0.6 to 0.7, is there any BC, how is it recommended to do the upgrade. Upgrading to 0.6.12 won't help me with anything as you say, because 0.7 serializes POJOs differently. Eventually in systems that are not monolith, doing such upgrade is not a trivial task - there's no changelog, no migration path. I'd really want to upgrade to latest version, but as it seems my only solution currently is taking msgpack-java code, trying to do some fixes that 0.7 will be compatible with 0.6, wait some time until all services will be deployed with new version, and only then use the official released version. If you see another way, I'd be happy if you could share.

marenzo avatar Jan 04 '16 15:01 marenzo

@marenzo We found Jackson annotation @JsonFormat(shape=JsonFormat.Shape.ARRAY). You could serialize and deserialize POJOs in both 0.6 and 0.7 using the annotation like this:

    @Message
    @JsonFormat(shape=JsonFormat.Shape.ARRAY)
    static class User {
        public String name;
        public int age;
        public float pi;

        @Override
        public String toString() {
            return "User{" + "name='" + name + '\'' + ", age=" + age + ", pi=" + pi + '}';
        }
    }

    public static void main(String[] args) throws IOException {
        User user = new User();
        user.name = "foobar";
        user.age = 42;;
        user.pi = 3.14f;

        MessagePack messagePack = new MessagePack();
        ObjectMapper mapper = new ObjectMapper(new MessagePackFactory());

        // w(0.6) -> r(0.7)
        {
            byte[] bytes = messagePack.write(user);
            System.out.println(mapper.readValue(bytes, User.class));
        }

        // w(0.7) -> r(0.6)
        {
            byte[] bytes = mapper.writeValueAsBytes(user);
            System.out.println(messagePack.read(bytes, User.class));
        }
    }

komamitsu avatar Jan 28 '16 02:01 komamitsu

@komamitsu sorry for late reply. Thanks for the answer, I've tried that and seems to work well! I've created a new PR about the other discussion of str8 support, and with that change I believe I'll be able to do this migration safe (#356).

marenzo avatar Apr 06 '16 15:04 marenzo

@xerial @komamitsu hey again. today I've found some strange case, where old msgPack fails to unserialize value of new serializer - when I'm trying to serialize Map of Integer -> Pojo with the new api, I'm failing to unserialize it from the old one. Check out the following test:

  // entity to check
  private BasicEntity entity;

  @Before
  public void initialize() {

    // pojo to test
    entity = new BasicEntity(1, "blabla", asList("abc"), singletonMap(5, "v"));

    // creating old msgpack client
    msgPack = new MessagePack();
    msgPack.register(BasicEntity.class);

    // creating new msgpack client (via ObjectMapper)
    objectMapper = new ObjectMapper(new MessagePackFactory(new PackerConfig().withStr8FormatSupport(false)));
    objectMapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true);
    objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
    objectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
    objectMapper.setAnnotationIntrospector(new JsonArrayFormat());
  }

  @Test
  public void pojoCompatibility() throws Exception {

    final byte[] oldBytes = msgPack.write(entity);
    final byte[] newBytes = objectMapper.writeValueAsBytes(entity);

//    assertArrayEquals("serializations should be the same", oldBytes, newBytes);

    final BasicEntity oldEntity = msgPack.read(newBytes, BasicEntity.class);
    final BasicEntity newEntity = objectMapper.readValue(oldBytes, BasicEntity.class);

    assertEquals("both entities should be the same", oldEntity, newEntity);
  }

  public static class BasicEntity {

    public int number;
    public String value;
    public List<String> values;
    public Map<Integer, String> map; // the problamatic map
  }

I'm having here a pojo called BasicEntity, which I'm putting in a map and trying to serialize / unserialize, and I'm failing on MessageTypeException: Unexpected raw value.

Also, when I'm wrapping a pojo inside a Map of Integer -> Pojo, I'm just getting MessageTypeException without any message.

Note the commented line - the byte arrays are not equal for some reason, but I want the test to fail on the unserialize part.

Any ideas?

marenzo avatar Apr 16 '16 22:04 marenzo

Could you show us the actual generated byte array (in hex format)?

xerial avatar Apr 16 '16 23:04 xerial

This is the generated byte array: old serializer: "9401a6626c61626c6195a176a162a162a166a1728105a176" new serializer: "9401a6626c61626c6195a176a162a162a166a17281a135a176"

the entity I'm serializing with its values located at the previous comment test

marenzo avatar Apr 17 '16 08:04 marenzo

One last thing, if I haven't mentioned, the new serializer does able to deserialize the byte array of the old one

marenzo avatar Apr 17 '16 08:04 marenzo

@marenzo jackson-dataformat-msgpack which is used in 0.7 is one of Jackson Databind extensions and Jackson Databind accepts only String as a key of JSON object. So the key of Map<Integer, String> is needed to be serialized as String and deserialized it as Integer according to its class definition in Jackson Databind (jackson-dataformat-msgpack).

But, MessagePack 0.6 doesn't know that and tries to deserialize the String key as Integer and fails...

komamitsu avatar Apr 17 '16 14:04 komamitsu

@komamitsu isn't we're loosing here a bit of msgpack compactness, where the protocol doesn't contain such limitation (comparing to Json), but here we're disabling this feature?

I can run some tests compare generated byte arrays sizes with the new client compare to the old one if you'd like

marenzo avatar Apr 17 '16 15:04 marenzo

on same topic, added one more test which does:

  @Test
  public void mapSerialization() throws IOException {

    final Map<BasicEntity, String> entitiesMap = singletonMap(new BasicEntity(123, "abc"), "value");
    final Type type = new TypeToken<Map<BasicEntity, String>>(){}.getType(); // just getting parametrized type
    final JavaType jacksonType = objectMapper.getTypeFactory().constructType(type);

    final byte[] bytes = objectMapper.writeValueAsBytes(entitiesMap);
    final Map<BasicEntity, String> result = objectMapper.readValue(bytes, jacksonType);

    assertEquals("both maps should be equal", entitiesMap, result);
  }

and failing on:

com.fasterxml.jackson.databind.JsonMappingException: Can not find a (Map) Key deserializer for type [simple type, class com.outbrain.ob1k.server.msgpack.MessagePackCompatibilityTest$BasicEntity]

as you mentioned, the JSON key has to be String, and also ObjectMapper doesn't know how to handle maps where the key is custom pojo, so (at least by what Google's says) you need to create your own key serializer / deserializer.

I think we can fix both of this issues by implementing by ourselves key serializer / un-serializer (if possible, of course). The default one is: StdKeySerializer

some reference: http://stackoverflow.com/questions/11246748/deserializing-non-string-map-keys-with-jackson

marenzo avatar Apr 17 '16 16:04 marenzo

Yeah, also com.fasterxml.jackson.core.SerializableString seems important. I hope I have time to handle the issue this week...

komamitsu avatar Apr 17 '16 17:04 komamitsu

@marenzo I added some changes to jackson-dataformat-msgpack to support non-string key. https://github.com/msgpack/msgpack-java/pull/361

You can use this feature with adding @JsonSerialize(keyUsing = MessagePackKeySerializer.class) annotation like https://github.com/msgpack/msgpack-java/blob/develop/msgpack-jackson/src/test/java/org/msgpack/jackson/dataformat/MessagePackGeneratorTest.java#L490-L503 or calling objectMapper.setSerializerFactory(new MessagePackSerializerFactory()) like https://github.com/msgpack/msgpack-java/blob/develop/msgpack-jackson/src/test/java/org/msgpack/jackson/dataformat/MessagePackGeneratorTest.java#L658. It's a bit experimental, though.

komamitsu avatar Apr 19 '16 16:04 komamitsu

Nice! my other tests are passing now, but the one I've provided (in my comment above) is still failing, with a new exception:

java.lang.IllegalArgumentException: com.outbrain.ob1k.server.msgpack.MessagePackCompatibilityTest$BasicEntity@e1781

    at org.msgpack.jackson.dataformat.MessagePackGenerator.pack(MessagePackGenerator.java:227)
    at org.msgpack.jackson.dataformat.MessagePackGenerator.packObject(MessagePackGenerator.java:268)
    at org.msgpack.jackson.dataformat.MessagePackGenerator.flush(MessagePackGenerator.java:463)
    at org.msgpack.jackson.dataformat.MessagePackGenerator.close(MessagePackGenerator.java:447)
    at com.fasterxml.jackson.databind.ObjectMapper._configAndWriteValue(ObjectMapper.java:3561)
    at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsBytes(ObjectMapper.java:2951)

When I'm having a map with a primitive as a key, works well. But when I'm having a map with a pojo key, it fails. But from this exception it looks like its failing because of some core issue.

marenzo avatar Apr 20 '16 17:04 marenzo

@marenzo jackson-dataformat-msgpack supports POJO key in a map object with https://github.com/msgpack/msgpack-java/issues/364. Can you try it?

Here is an example https://github.com/msgpack/msgpack-java/blob/develop/msgpack-jackson/src/test/java/org/msgpack/jackson/dataformat/MessagePackGeneratorTest.java#L716-L734

komamitsu avatar Apr 26 '16 17:04 komamitsu

@komamitsu sorry, but now I'm getting again the previous exception:

com.fasterxml.jackson.databind.JsonMappingException: Can not find a (Map) Key deserializer for type [simple type, class com.outbrain.ob1k.server.msgpack.MessagePackCompatibilityTest$BasicEntity]

You're a bit cheating with tests on your latest commits since the serialization is done via ObjectMapper, but you're de-serializing the byte array directly with MessageUnpacker :)

You can find my tests here: https://github.com/outbrain/ob1k/blob/msgpack_upgrd/ob1k-core/src/test/java/com/outbrain/ob1k/server/msgpack/MessagePackCompatibilityTest.java (the failing one is complexTypeMapCompatibility), or you can re-produce it with the test few comments above (pretty much the same one)

I'll try to give it a look later this week. Thanks for the helping so far!

marenzo avatar May 04 '16 21:05 marenzo

Well, I don't understand why you said "cheating" on the test (https://github.com/msgpack/msgpack-java/blob/develop/msgpack-jackson/src/test/java/org/msgpack/jackson/dataformat/MessagePackGeneratorTest.java#L716-L734). The test is definitely for complex type key serialization in v0.8 not for deserialization.

As for the previous test you pasted, I found the serialization of complex type key in v0.8 didn't work well before deserialization. I thought to fix the issue might be helpful for you if you deserialize complex type key data in v0.6 which is serialized in v0.8.

I originally don't think it's possible that v0.8 deserializes complex type key for now because deserialization in jackson-databind doesn't accept SerializedString while Serialization accepts SerializedString.

BTW, let me clarify my position. I don't recommend to use v0.6 since it serializes POJO in a naive way depending on JVM implementation, let alone its poor performance. Also, the serealization and deserialization of POJO in v0.6 don't have compatibility with MessagePack libraries written in other languages. And, we didn't guarantee 100% compatibility with v0.6 because we chose the stability and features of jackson-databind.

komamitsu avatar May 05 '16 09:05 komamitsu

@komamitsu

It's possible. After reading those links: http://wiki.fasterxml.com/JacksonFeatureModules http://wiki.fasterxml.com/JacksonHowToCustomDeserializers I've basically created my own "MessagePackModule" with a Map deserializer implementation (extending StdDeserializer), and I was able to deserialize a Map where's the key was an entity (such as Map<Pojo, String>)

But, after I wrote the code, I've discovered new issue: the value is being serialized always as either String or Array, so primitive values (int, bool, etc..) are failing on deserialization. This issue exists both in 0.6 and 0.8. For example, serializing Map<Pojo, Integer> will result in byte array of array & string, rather than Integer.

Because of this, I think we're good enough now. If the previous client haven't supported such case well, the new client also shouldn't try. If I'll find more issues, I'll update.

Thanks for the help!

marenzo avatar May 05 '16 23:05 marenzo

@marenzo

my own "MessagePackModule" with a Map deserializer implementation (extending StdDeserializer)

Cool. Can I see it somewhere?

komamitsu avatar May 06 '16 06:05 komamitsu

@komamitsu sure. https://github.com/marenzo/msgpack-java/commit/d28f70cb34a2f2f8bc8d78c496e544b42190014d

the code isn't very polished or optimized, and I'm using directly objectMapper inside the deserializer as I'm not familiar enough with Jackson / MessagePackParser / etc, I've tried with this code to see if it's even possible to do such thing.

note the testMapWithPrimitiveValue test - the reason it fails because for some reason it considers the value as a String, and causing error when trying to unmarshall to integer. Same thing happens with 0.6 client, but I haven't digged too much, maybe you know why :)

marenzo avatar May 06 '16 20:05 marenzo

I'm stopping by to say a huge THANK YOU to everyone who made str8FormatSupport possible. I'm in the process of upgrading from an old 0.6 to latest 0.8 across multiple components in our service and this will make my life so much easier.

thank you

lmarburger avatar Dec 11 '17 19:12 lmarburger