Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move msgpack value classes to another project msgpack-value #571

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dmikurube
Copy link

@dmikurube dmikurube commented May 18, 2021

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 dmikurube mentioned this pull request May 18, 2021
8 tasks
@xerial
Copy link
Member

xerial commented May 18, 2021

@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

@dmikurube
Copy link
Author

@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:

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

@xerial
Copy link
Member

xerial commented May 21, 2021

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

Yes, unless there are no critical bugs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants