Add org.embulk.spi.json.JsonValue to deprecate use of msgpack-core in embulk-api#1462
Merged
Add org.embulk.spi.json.JsonValue to deprecate use of msgpack-core in embulk-api#1462
Conversation
1c3878d to
8838d56
Compare
964c80a to
5744d02
Compare
5744d02 to
1b715a4
Compare
529548a to
022a0cf
Compare
3d4bfca to
bbc5a16
Compare
bbc5a16 to
17ccd8a
Compare
hiroyuki-sato
approved these changes
Jan 17, 2023
Member
hiroyuki-sato
left a comment
There was a problem hiding this comment.
LGTM 👍 I left minor comments.
Member
Author
|
It's corresponding EEP is underway: #1538 This pull request is just adding new classes, which does not affect any existing things yet. Ket me merge this for further work although we'll still need to discuss the design with the forthcoming EEP. We may revert it or modify it later if we get some different conclusion from the EEP discussions. |
Member
Author
|
Thanks for reviewing, @hiroyuki-sato. Merging this. (Cc: @vietnguyen-td) |
This was referenced Jan 18, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Embulk's "JSON"-type values have been represented by msgpack-java's
org.msgpack.value.(Immutable)Valuesince Embulk v0.7. The msgpack-java has set its roots deep inside Embulk, including its plugin SPI method signatures.It has brought a heavy dependency hell between Embulk and msgpack-java. We have hesitated even just upgrading
msgpack-corewhich can potentially cause a compatibility issue. As a result, a couple of plugins that usemsgpack-core(such asembulk-parser-msgpack) had to stay with oldermsgpack-core.We upgraded
msgpack-coreto 0.8.24 in #1459, in the Embulk v0.10 "development" series.However, indeed, the Embulk core has never used
MessagePackernorMessageUnpackerfrommsgpack-core. Embulk has used onlyorg.msgpack.value.(Immutable)Valuejust as a JSON-like data container. It's nonsense!Then, we plan to drop
msgpack-corefrom the Embulk API/SPI in a long term. The plan would be:msgpack-coreto v0.8.24 in Embulk v0.10.42 in the pull request Upgrade msgpack-core to 0.8.24 #1459.org.embulk.spi.json.JsonValue) in v0.10.42 in this pull request Add org.embulk.spi.json.JsonValue to deprecate use of msgpack-core in embulk-api #1462.org.msgpack.value.Valueandorg.embulk.spi.json.JsonValue) for a long time -- even after Embulk v1.0.org.embulk.spi.time.Timestampandjava.time.Instant.org.embulk.spi.json.JsonValuethrough Embulk v0.11 - v1.0.org.embulk.spi.json.*classes to represent their internal values by msgpack-core classes again. It will allow to keep backward-compatible API/SPI methods for a while.org.embulk.spi.json.Json*also in Embulk v0.10.42. In a long term, plugins will have to migrate to use the new methods, instead ofmsgpack-core-based methods.msgpack-coreand API/SPI methods usingmsgpack-corewill be removed.We'll also write up an EEP for this.