abstractdog commented on code in PR #474:
URL: https://github.com/apache/tez/pull/474#discussion_r3378361333
##########
tez-api/src/test/java/org/apache/tez/dag/api/client/TestTimelineReaderFactory.java:
##########
@@ -52,10 +52,10 @@ public void
testPseudoAuthenticatorConnectionUrlShouldHaveUserName() throws Exce
ConnectionConfigurator connConf = mock(ConnectionConfigurator.class);
TimelineReaderPseudoAuthenticatedStrategy.PseudoAuthenticatedURLConnectionFactory
connectionFactory = new TimelineReaderPseudoAuthenticatedStrategy
- .PseudoAuthenticatedURLConnectionFactory(connConf);
+ .PseudoAuthenticatedURLConnectionFactory(connConf);
String inputUrl = "http://host:8080/path";;
String expectedUrl = inputUrl + "?user.name=" +
UserGroupInformation.getCurrentUser().getShortUserName();
- HttpURLConnection httpURLConnection =
connectionFactory.getHttpURLConnection(new URL(inputUrl));
+ HttpURLConnection httpURLConnection =
connectionFactory.getConnection(URI.create(inputUrl).toURL());
Review Comment:
this is not related to the hadoop upgrade I think, but instead `new URL`
becoming deprecated from Java20
if so, please create a separate ticket for handling the same in the whole
tez codebase:
```
grep -iRH "new URL" --include="*.java"
```
##########
tez-api/src/main/java/org/apache/tez/dag/api/client/TimelineReaderFactory.java:
##########
@@ -221,17 +221,17 @@ public
TokenAuthenticatedURLConnectionFactory(ConnectionConfigurator connConfigu
}
@Override
- public HttpURLConnection getHttpURLConnection(URL url) throws
IOException {
+ public HttpURLConnection getConnection(URL url) throws IOException {
try {
- AuthenticatedURL authenticatedURL=
ReflectionUtils.createClazzInstance(
+ Object authenticatedURL = ReflectionUtils.createClazzInstance(
DELEGATION_TOKEN_AUTHENTICATED_URL_CLAZZ_NAME, new Class[] {
delegationTokenAuthenticatorClazz,
ConnectionConfigurator.class
}, new Object[] {
authenticator,
connConfigurator
});
- return ReflectionUtils.invokeMethod(authenticatedURL,
+ return (HttpURLConnection)
ReflectionUtils.invokeMethod(authenticatedURL,
Review Comment:
cast is not needed here
##########
tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/parser/SimpleHistoryParser.java:
##########
@@ -255,8 +255,8 @@ protected void readEventsFromSource(String dagId,
JSONObjectSource source,
JSONObject subEntity = relatedEntities.getJSONObject(i);
String subEntityType = subEntity.optString(Constants.ENTITY_TYPE);
if (subEntityType != null && subEntityType.equals(Constants.USER))
{
- userName = subEntity.getString(Constants.ENTITY);
- break;
+ userName = subEntity.getString(Constants.ENTITY_ID);
+ break;
Review Comment:
this seems unnecessary, we indent 2 spaces
it's strange that checkstyle didn't warn about this, can you please create a
dummy PR to validate the same and a Jira if needed?
##########
tez-plugins/tez-yarn-timeline-history-with-acls/src/test/java/org/apache/tez/dag/history/ats/acls/TestATSHistoryWithACLs.java:
##########
@@ -148,23 +150,38 @@ public static void tearDown() throws InterruptedException
{
// To be replaced after Timeline has java APIs for domains
private <K> K getTimelineData(String url, Class<K> clazz) {
- Client client = new Client();
- WebResource resource = client.resource(url);
-
- ClientResponse response = resource.accept(MediaType.APPLICATION_JSON)
- .get(ClientResponse.class);
+ Client client = ClientBuilder.newClient();
+ WebTarget target = client.target(url);
+ Response response = target.request(MediaType.APPLICATION_JSON).get();
assertEquals(200, response.getStatus());
-
assertTrue(MediaType.APPLICATION_JSON_TYPE.isCompatible(response.getType()));
-
- JSONObject entity = response.getEntity(JSONObject.class);
- K converted = null;
+
assertTrue(MediaType.APPLICATION_JSON_TYPE.isCompatible(response.getMediaType()));
+ String entityStr = response.readEntity(String.class);
try {
- converted = convertJSONObjectToTimelineObject(entity, clazz);
+ JSONObject jsonObject = new JSONObject(entityStr);
+ JSONObject effectiveJson;
+ // Handle the nesting introduced by Jersey 2/Jackson
+ if (clazz == TimelineDomain.class) {
+ // TimelineDomain is annotated @XmlRootElement(name="domain"), Jersey
2/Jackson wraps it
+ effectiveJson = jsonObject.has("domain")
+ ? jsonObject.getJSONObject("domain")
+ : jsonObject;
+ } else if (clazz == TimelineEntity.class) {
+ // TimelineEntity is annotated @XmlRootElement(name="entity"), Jersey
2/Jackson wrap it
Review Comment:
nit: `wraps`
##########
tez-api/src/main/java/org/apache/tez/dag/api/client/TimelineReaderFactory.java:
##########
@@ -221,17 +221,17 @@ public
TokenAuthenticatedURLConnectionFactory(ConnectionConfigurator connConfigu
}
@Override
- public HttpURLConnection getHttpURLConnection(URL url) throws
IOException {
+ public HttpURLConnection getConnection(URL url) throws IOException {
try {
- AuthenticatedURL authenticatedURL=
ReflectionUtils.createClazzInstance(
+ Object authenticatedURL = ReflectionUtils.createClazzInstance(
Review Comment:
I believe this should work with AuthenticatedURL
##########
pom.xml:
##########
@@ -822,6 +819,19 @@
</dependencies>
</dependencyManagement>
+ <dependencies>
Review Comment:
why `<dependencies>` appeared here? afaik, we only handle
`dependencyManagement` in the root pom.xml
--
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]
