HADOOP-19231. Add JacksonUtil #6953
Merged
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
github-actions
bot
added
YARN
MapReduce
HDFS
trunk
Common
TOOLS
ABFS
DYNAMOMETER
labels
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
pjfanning
force-pushed
the
HADOOP-19231-jackson
branch
from
8edf3db to
7e1efdc
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
pjfanning
force-pushed
the
HADOOP-19231-jackson
branch
from
86dfb30 to
a4befb7
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@ayushtkn @slfan1989 @steveloughran Would any of you have time to review this? |
There was a problem hiding this comment.
Looks good. My main concern is about to get that import placement right; we should move it into the org.apache block now. The fact we didn't do this for the Preconditions migration is it something I have mixed feelings about.
bad: keep putting new entries in the wrong place
good: marginally easier to cherrypick into branches which have not moved to the shaded stuff.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JacksonUtil.java
Show resolved
Hide resolved
| import com.fasterxml.jackson.core.JsonGenerator; | ||
| import org.apache.hadoop.util.JacksonUtil; |
There was a problem hiding this comment.
move into org.apache block
...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/MetricsJsonBuilder.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JacksonUtil.java
Outdated
Show resolved
Hide resolved
...ct/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/util/CombinedHostsFileReader.java
Show resolved
Hide resolved
| import com.fasterxml.jackson.databind.ObjectReader; | ||
| import org.apache.hadoop.util.JacksonUtil; |
There was a problem hiding this comment.
this should go below. that Preconditions entry is in the wrong place due to the move off guava being a search and replace
| @@ -22,7 +22,7 @@ | |||
| import java.nio.charset.StandardCharsets; | |||
| import java.util.Map; | |||
|
|
|||
| import com.fasterxml.jackson.databind.ObjectMapper; | |||
| import org.apache.hadoop.util.JacksonUtil; | |||
| @@ -29,9 +29,9 @@ | |||
| import java.util.Hashtable; | |||
| import java.util.Map; | |||
|
|
|||
| import org.apache.hadoop.util.JacksonUtil; | |||
There was a problem hiding this comment.
how about moving both of these?
| @@ -46,6 +47,19 @@ | |||
| public class YarnServiceClient { | |||
|
|
|||
| private static final Logger LOG = LoggerFactory.getLogger(YarnServiceClient.class); | |||
|
|
|||
| /** | |||
| * It is more performant to reuse ObjectMapper instances but keeping the instance | |||
There was a problem hiding this comment.
not 100% sure about that performant: isn't there some synchronisation locking going on so performance with a single mapper across many threads worse than 1 mapper per thread?
There was a problem hiding this comment.
I am a committer on the Jackson project. There is minimal synchronisation and I am actively removing many of the remaining cases. Even today, it is more performant to reuse ObjectMapper instead of creating multiple instances.
There was a problem hiding this comment.
are you? never knew that.
fix checkstyle
checkstyle issues
pjfanning
force-pushed
the
HADOOP-19231-jackson
branch
from
a8c72f9 to
da58faf
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
...p-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/RemoteSASKeyGeneratorImpl.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JacksonUtil.java
Show resolved
Hide resolved
| @@ -46,6 +47,19 @@ | |||
| public class YarnServiceClient { | |||
|
|
|||
| private static final Logger LOG = LoggerFactory.getLogger(YarnServiceClient.class); | |||
|
|
|||
| /** | |||
| * It is more performant to reuse ObjectMapper instances but keeping the instance | |||
There was a problem hiding this comment.
are you? never knew that.
|
💔 -1 overall
This message was automatically generated. |
|
merged to trunk...can you do a branch-3.4 PR? |
pjfanning
added a commit
to pjfanning/hadoop
that referenced
this pull request
Aug 15, 2024
New class org.apache.hadoop.util.JacksonUtil centralizes construction of Jackson ObjectMappers and JsonFactories. Contributed by PJ Fanning merge issues
4 tasks
|
#6998 created |
|
@steveloughran / @pjfanning is bit misleading, since it looses the actual exception, because we of a NPE during cleanup, but if you comment that NPE line & related ones, the exception seems to be this: |
|
@ayushtkn The strange thing is that org.apache.hadoop.yarn.util.timeline.TimelineUtils imports another class from the same hadoop-common package as JacksonUtil. import org.apache.hadoop.util.VersionInfo; Is it possible that this CI task is another an older copy of hadoop-common jar? |
|
The HBase stuff is bit tricky, for the HBase test we use the older Hadoop version, which is compatible with the HBase release we are using. It is hardcoded here: I think this problem is bit similar to YARN-8856 |
|
so what to do? revert?
|
|
The failing tests run with hadoop-common 3.3.6 and the JacksonUtil is not in 3.3.6. |
4 tasks
|
@pjfanning This pr brought some unit test errors. HBaseTimelineWriterImpl#testDomainIdTable TestTimelineReaderWebServicesHBaseStorage#setup https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6977/10/testReport/ |
Have readed the context and understood the relevant reasons. Continuously encountering errors is not a good option; perhaps we should roll back first? |
4 tasks
|
I created #7010 |
|
I think we don't have an agreement on how to proceed here, we should most probably rollback considering the failing tests and rework later. lemme know if anyone has objections doing so, else will revert tmrw |
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
New class org.apache.hadoop.util.JacksonUtil centralizes construction of Jackson ObjectMappers and JsonFactories. Contributed by PJ Fanning
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
New class org.apache.hadoop.util.JacksonUtil centralizes construction of Jackson ObjectMappers and JsonFactories. Contributed by PJ Fanning
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.
Description of PR
HADOOP-19231
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?