joseluisll commented on code in PR #8841:
URL: https://github.com/apache/hadoop/pull/8841#discussion_r4428642640
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/ApplicationHistoryServer.java:
##########
@@ -371,7 +369,7 @@ protected ResourceConfig configure() {
config.register(TimelinePutResponseReader.class);
config.register(new JerseyBinder());
config.register(GenericExceptionHandler.class);
- config.register(new
JettisonFeature()).register(YarnJacksonJaxbJsonProvider.class);
+ config.register(YarnJacksonJaxbJsonProvider.class);
Review Comment:
Fixed in 8d2f4d6a301. `YarnJacksonJaxbJsonProvider` now builds its mappers
once and uses a separate one for `TimelineAbout`, so it no longer changes a
mapper that is in use.
Two details turned out differently when I ran it. Jackson caches a writer
per type and annotations, so the leak only hits types first written after
`/about`. That means "call `/about`, then post" fails only if `/about` is the
very first request, and an earlier put on a shared server would hide it.
`TestYarnJacksonJaxbJsonProvider` uses a new provider for that reason. Also,
the reader does not throw: `TimelinePutResponse` ignores unknown fields, so the
client saw zero errors on a denied put.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/ApplicationHistoryServer.java:
##########
@@ -360,7 +359,6 @@ protected ResourceConfig configure() {
ResourceConfig config = new ResourceConfig();
config.packages("org.apache.hadoop.yarn.server.timeline.webapp");
config.packages("org.apache.hadoop.yarn.server.applicationhistoryservice.webapp");
- config.packages("org.apache.hadoop.yarn.api.records.writer");
config.register(TimelineWebServices.class);
config.register(AHSWebServices.class);
config.register(TimelineEntitiesWriter.class);
Review Comment:
Agreed, and it goes beyond the shape difference: a `POST` using the
documented `entity` and `entitytype` names gets HTTP 400. I have left it out of
this PR because `TimelineEntitiesWriter` is also what the v1 client,
`TimelineWriter`, posts with, so the fix needs a compatibility decision of its
own. It is tracked in
[YARN-12052](https://issues.apache.org/jira/browse/YARN-12052), and the PR
description now lists entity GETs as a known gap.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/ApplicationHistoryServer.java:
##########
@@ -360,7 +359,6 @@ protected ResourceConfig configure() {
ResourceConfig config = new ResourceConfig();
config.packages("org.apache.hadoop.yarn.server.timeline.webapp");
config.packages("org.apache.hadoop.yarn.server.applicationhistoryservice.webapp");
Review Comment:
Done in 8d2f4d6a301, both scans are removed. I checked that the only `@Path`
or `@Provider` classes in those packages are `TimelineWebServices` and
`AHSWebServices`, which are registered explicitly, and that there are no
subpackages.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/webapp/TestTimelineWebServices.java:
##########
@@ -118,7 +117,7 @@ protected Application configure() {
config.register(TimelineEventsWriter.class);
config.register(GenericExceptionHandler.class);
config.register(TimelinePutResponseWriter.class);
- config.register(new
JettisonFeature()).register(YarnJacksonJaxbJsonProvider.class);
+ config.register(YarnJacksonJaxbJsonProvider.class);
Review Comment:
Done in 8d2f4d6a301, this config now registers the same list as
`ApplicationHistoryServer.configure()`. Removing those writers exposed a real
bug: the server writes `entity`, `entitytype` and `errorcode`, but the client
reader only knew the camelCase names, so `testPostEntitiesWithYarnACLsEnabled`
got error code 0 instead of 4. `TimelinePutError` now accepts both spellings
through `@JsonAlias`. The test clients also read events and domains with the
JAXB names, as the server writes them.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/applicationhistoryservice/webapp/JAXBContextResolver.java:
##########
@@ -1,61 +0,0 @@
-/**
Review Comment:
I have kept the removal. Neither the class nor its package carries an
audience annotation, so `Compatibility.md` treats it as Private and Unstable,
and nothing in the repository uses it. It is mentioned in the release note on
YARN-12030.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
