HADOOP-16488 Deprecated JsonSerialization and move it out of hadoop-c… #1222
Conversation
This file contains 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
|
Ping @steveloughran @jojochuang . This is the first try. Not finished yet. The approach is to introduce a GsonSerialization, and use it to replace the JsonSerialization. And now the difficulty is that, we use the jackson annotation in many places, so I still need sometime to find out how to deal with these annotations. And for hadoop-aws, maybe we can just move copy some code for JsonSerialization there, as the java sdk for aws depends on jackson so it is useless to purge the jackson dependency for it. |
hadoop-yetus
reviewed
Aug 4, 2019
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
|
|
||
| import static org.apache.commons.lang3.StringUtils.isBlank; | ||
| import static org.apache.commons.lang3.StringUtils.isNotBlank; | ||
|
|
There was a problem hiding this comment.
your IDE is moving imports around which is shouldn't; this always makes backporting of patches to a key class harder. Can you leave out all changes to the imports except for those classes added/removed
There was a problem hiding this comment.
Is there an import order rule? I checked several files but can not find the pattern, static import can be anywhere, and no alphabetical order...
| dumpGenerator.writeStartArray(); | ||
| dumpGenerator.flush(); | ||
| JsonWriter jsonWriter = new JsonWriter(out); | ||
| jsonWriter.beginObject().name("properties").beginArray().flush(); |
There was a problem hiding this comment.
do we have tests for this writing of configs? If not, they're needed first
There was a problem hiding this comment.
We have, but as you said below, if we purge jackson completely, there is no way to ensure that the json format is not changed, as in tests we also use gson...
AWS SDK is shaded; their problem. We need to get hadoop-aws off it, which is doable. Hadoop-registry is the origin of this code; need to review that to make sure the records persistet can be marshalled without problems For all the changes, I think we should add tests verifying consistent loading of existing classes first. That is: we save the current .json files as test resources and verify that they can be loaded with the new code. That's a good practise anyway, isn't it? |
This seems to be a good idea. Will have a try after I finish purging the jackson stuff. Thanks. |
hadoop-yetus
reviewed
Aug 8, 2019
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
hadoop-yetus
reviewed
Aug 8, 2019
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
|
💔 -1 overall
This message was automatically generated. |
| @@ -18,14 +18,18 @@ | |||
|
|
|||
| package org.apache.hadoop.conf; | |||
|
|
|||
| import static org.apache.commons.lang3.StringUtils.isBlank; | |||
There was a problem hiding this comment.
these should go at the bottom; imports should be as unchanged as possible
|
|
||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
| import java.util.regex.PatternSyntaxException; |
There was a problem hiding this comment.
and similarly, dont reorder stuff even when its wrong...it makes backporting too hard
| @@ -57,9 +57,13 @@ | |||
| * {@code org.apache.hadoop.registry.client.binding.JsonSerDeser}, | |||
| * which is now a subclass of this class. | |||
| * @param <T> Type to marshal. | |||
| * @deprecated Avoid using this class any more, as jackson has | |||
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…ommon