pjfanning commented on code in PR #8841:
URL: https://github.com/apache/hadoop/pull/8841#discussion_r4400672959
##########
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:
With Jettison gone, `YarnJacksonJaxbJsonProvider` writes everything not
covered by a dedicated writer, so its `locateMapper()` now matters on every
response:
```java
ObjectMapper mapper = super.locateMapper(type, mediaType);
configObjectMapper(mapper);
if (type == TimelineAbout.class) {
mapper.enable(SerializationFeature.WRAP_ROOT_VALUE);
}
```
- `WRAP_ROOT_VALUE` is enabled on the shared mapper and never disabled.
After the first `GET /ws/v1/timeline/about` (or
`/ws/v1/applicationhistory/about`), every response is root-wrapped, e.g. put
returns `{"response":{"errors":[]}}`, which `TimelinePutResponseReader` rejects
with `UnrecognizedPropertyException`.
- `configObjectMapper()` re-sets the introspector and inclusion on the
singleton mapper per request, which races with concurrent writes.
I think this needs fixing in the provider as part of (or before) this
change. Otherwise the shape still depends on request history. A test that calls
`/about` first and then posts an entity would catch it.
##########
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:
The description avoids pointing the scan at `records.timeline.writer`
because those writers use a plain `ObjectMapper` and would add a third shape,
but `TimelineEntitiesWriter` and `TimelineEntityWriter` are still registered
here. So `GET /ws/v1/timeline/{type}` and `/{type}/{id}` are already written
without the JAXB introspector and with nulls included, unlike
domains/events/put. Either have these writers use a JAXB-configured mapper, or
drop them and let the provider handle entities, so there really is one shape.
##########
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:
With `JAXBContextResolver` deleted, these two scans no longer find anything
that isn't already registered explicitly (`TimelineWebServices`,
`AHSWebServices`). They cost a classpath scan at startup and would silently
pick up any `@Provider` added to these packages later, which is how the
resolver ended up affecting the output. Could they be dropped like the
`records.writer` one?
##########
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:
Minor: this is a public class in `src/main` with no `@Private`/`@Unstable`
annotation. Removing it outright could break anything downstream that registers
it. Probably fine for a webapp-internal class, but worth mentioning in the
release note (or deprecating first if we want to be strict).
##########
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:
This still differs from production: the test registers
`TimelineEventsWriter` and `TimelinePutResponseWriter` (above), which
`ApplicationHistoryServer.configure()` does not. In production, events and put
responses are written by `YarnJacksonJaxbJsonProvider`, including the
post-`/about` root wrapping, so this test can't catch regressions on the
production path. Consider making the test config match `configure()` exactly
(or reusing it).
--
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]
