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

Move msgpack value classes to another project msgpack-value

Open dmikurube opened this issue 4 years ago • 3 comments

Some org.msgpack.value classes (typically org.msgpack.value.Value) are used in method signatures of other software's API/SPI, unfortunately. https://dev.embulk.org/embulk-api/0.10.31/javadoc/org/embulk/spi/PageBuilder.html#setJson-org.embulk.spi.Column-org.msgpack.value.Value-

Against such cases, I was wondering if those value classes are separated in a different Maven artifact so that API/SPI includes only stable msgpack-value, and implementation is released from dependency hell...

Can I hear your thoughts?

I don't stick with the implementation shown in this PR, such as naming, class structures, and else. This PR is just as a basis for discussion. But such a separation is really helpful.

dmikurube avatar May 18 '21 09:05 dmikurube

@dmikurube My preference order is: First, release Timestamp support without introducing such interface changes like this PR. msgpack-java has been carefully implemented so as not to use any TypeProfile, which will be generated when extending classes (e.g., implementing interfaces, extending abstract classes). In an old benchmark, extending any classes had non-negligible overhead. I'm not sure this result still holds in modern JVM, though. The next msgpack-java version would be 0.9.0.

If we release a next msgpack-java version and the performance difference is negligible, I'd like to simplify all of the interfaces. by changing the value classes to always use immutable values, and discard existing immutable value interfaces, which is just adding complexity to the entire code base. Then, it would be possible to make the value interfaces more stable. For example, creating msgpack-spi module and introducing org.msgpack.spi package. Value classes will be org.msgpack.spi.Value.

If we will introduce such breaking changes, I'm not sure how valuable it is to split the msgpack-value as a separate module at this moment as embulk SPI will be broken again by such a change.

I don't think we can create org.msgpack.spi soon, so a reasonable release plan would be like this:

  • 0.9.0 adds TimestampValue
  • 0.10.0 split value interface like this PR. Adding JMH based benchmark would be good.
  • 0.11.0 (This might be a year later or really quick if I have a chance to work on it) introduce org.msgpack.spi and separate packer/unpacker/value impl.
  • 1.0.0 finalize

xerial avatar May 18 '21 14:05 xerial

@xerial Thanks. Got it. I understand your concerns and plan.

Then, I think we'll make a choice of removing msgpack-java's classes from Embulk's API/SPI in a (very) long-term. Our plan would be:

  • Before Embulk v0.11.0 (expected to be Embulk v0.10.35), we upgrade msgpack-core in embulk-api to 0.8.24
    • https://github.com/embulk/embulk/pull/1459
    • Leaving a comment that Embulk is going to remove msgpack-core from embulk-api's dependencies.
    • Q: msgpack-core:0.8.24 is expected to be the last 0.8?
  • Then, we'll introduce a workaround class org.embulk.spi.json.JsonValue to encapsulate org.msgpack.value.Value.
    • https://github.com/embulk/embulk/pull/1462

The removal in Embulk API will be much much later to wait for plugins to catch up with the latest manner. But, I believe that making a clear declaration during v0.10 would be reasonable and making things smoother.

P.S. You may be interested in this: https://gist.github.com/dmikurube/c48ac6432cb4c2151ff9c75cd253bcd5

dmikurube avatar May 20 '21 09:05 dmikurube

Q: msgpack-core:0.8.24 is expected to be the last 0.8?

Yes, unless there are no critical bugs

xerial avatar May 21 '21 04:05 xerial