You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tez.apache.org by "abstractdog (via GitHub)" <gi...@apache.org> on 2026/05/18 06:56:01 UTC
Re: [PR] TEZ-4688 TEZ-4648: Tez upgrade to Hadoop 3.5.0 and jersey 2.x [tez]
abstractdog commented on code in PR #474:
URL: https://github.com/apache/tez/pull/474#discussion_r3256768154
##########
tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/ATSImportTool.java:
##########
@@ -279,76 +268,56 @@ private void downloadJSONArrayFromATS(String url, ZipOutputStream zos, String ta
//Set the last item in entities as the fromId
url = baseUrl + "&fromId="
- + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
- String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY);
- String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY_ID);
+ String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
LOG.info("Downloaded={}, First item={}, LastItem={}, new url={}", downloadedCount,
firstItem, lastItem, url);
}
}
- private void logErrorMessage(ClientResponse response) throws IOException {
- LOG.error("Response status={}", response.getClientResponseStatus().toString());
- LineIterator it = null;
+ private void logErrorMessage(Response response) {
+ LOG.error("Response status={}", Integer.toString(response.getStatus()));
Review Comment:
it's strange that we're logging normal messages on the error level (I know this wasn't introduced by this PR), what about moving to INFO?
##########
tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/ATSImportTool.java:
##########
@@ -279,76 +268,56 @@ private void downloadJSONArrayFromATS(String url, ZipOutputStream zos, String ta
//Set the last item in entities as the fromId
url = baseUrl + "&fromId="
- + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
- String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY);
- String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY_ID);
+ String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
LOG.info("Downloaded={}, First item={}, LastItem={}, new url={}", downloadedCount,
firstItem, lastItem, url);
}
}
- private void logErrorMessage(ClientResponse response) throws IOException {
- LOG.error("Response status={}", response.getClientResponseStatus().toString());
- LineIterator it = null;
+ private void logErrorMessage(Response response) {
+ LOG.error("Response status={}", Integer.toString(response.getStatus()));
try {
- it = IOUtils.lineIterator(response.getEntityInputStream(), UTF8);
- while (it.hasNext()) {
- String line = it.nextLine();
- LOG.error(line);
- }
- } finally {
- if (it != null) {
- it.close();
+ String entity = response.readEntity(String.class);
+ if (entity != null) {
+ LOG.error(entity);
Review Comment:
`INFO` here too
##########
tez-plugins/tez-yarn-timeline-history-with-acls/src/test/java/org/apache/tez/dag/history/ats/acls/TestATSHistoryWithACLs.java:
##########
@@ -148,23 +150,25 @@ 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);
+ // Handle the nesting introduced by Jersey 2/Jackson
+ JSONObject effectiveJson = jsonObject.has("domain") ? jsonObject.getJSONObject("domain") : jsonObject;
Review Comment:
do we need both branches after the upgrade?
##########
tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/ATSImportTool.java:
##########
@@ -279,76 +268,56 @@ private void downloadJSONArrayFromATS(String url, ZipOutputStream zos, String ta
//Set the last item in entities as the fromId
url = baseUrl + "&fromId="
- + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
- String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY);
- String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY_ID);
+ String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
LOG.info("Downloaded={}, First item={}, LastItem={}, new url={}", downloadedCount,
firstItem, lastItem, url);
}
}
- private void logErrorMessage(ClientResponse response) throws IOException {
- LOG.error("Response status={}", response.getClientResponseStatus().toString());
- LineIterator it = null;
+ private void logErrorMessage(Response response) {
+ LOG.error("Response status={}", Integer.toString(response.getStatus()));
try {
- it = IOUtils.lineIterator(response.getEntityInputStream(), UTF8);
- while (it.hasNext()) {
- String line = it.nextLine();
- LOG.error(line);
- }
- } finally {
- if (it != null) {
- it.close();
+ String entity = response.readEntity(String.class);
+ if (entity != null) {
+ LOG.error(entity);
}
+ } catch (Exception ignore) {
+ // ignore
Review Comment:
unless we know an obvious reason why to suppress it, we should at least `DEBUG`
##########
tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/ATSImportTool.java:
##########
@@ -279,76 +268,56 @@ private void downloadJSONArrayFromATS(String url, ZipOutputStream zos, String ta
//Set the last item in entities as the fromId
url = baseUrl + "&fromId="
- + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ + entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
- String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY);
- String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY);
+ String firstItem = entities.getJSONObject(0).getString(Constants.ENTITY_ID);
+ String lastItem = entities.getJSONObject(entities.length() - 1).getString(Constants.ENTITY_ID);
LOG.info("Downloaded={}, First item={}, LastItem={}, new url={}", downloadedCount,
firstItem, lastItem, url);
}
}
- private void logErrorMessage(ClientResponse response) throws IOException {
- LOG.error("Response status={}", response.getClientResponseStatus().toString());
- LineIterator it = null;
+ private void logErrorMessage(Response response) {
+ LOG.error("Response status={}", Integer.toString(response.getStatus()));
try {
- it = IOUtils.lineIterator(response.getEntityInputStream(), UTF8);
- while (it.hasNext()) {
- String line = it.nextLine();
- LOG.error(line);
- }
- } finally {
- if (it != null) {
- it.close();
+ String entity = response.readEntity(String.class);
+ if (entity != null) {
+ LOG.error(entity);
}
+ } catch (Exception ignore) {
+ // ignore
}
}
//For secure cluster, this should work as long as valid ticket is available in the node.
private JSONObject getJsonRootEntity(String url) throws TezException, IOException {
try {
- WebResource wr = getHttpClient().resource(url);
- ClientResponse response = wr.accept(MediaType.APPLICATION_JSON_TYPE)
- .type(MediaType.APPLICATION_JSON_TYPE)
- .get(ClientResponse.class);
+ WebTarget target = getHttpClient().target(url);
+ Response response = target.request(MediaType.APPLICATION_JSON_TYPE)
+ .accept(MediaType.APPLICATION_JSON_TYPE)
+ .get();
- if (response.getClientResponseStatus() != ClientResponse.Status.OK) {
+ if (response.getStatus() != Response.Status.OK.getStatusCode()) {
// In the case of secure cluster, if there is any auth exception it sends the data back as
// a html page and JSON parsing could throw exceptions. Instead, get the stream contents
// completely and log it in case of error.
logErrorMessage(response);
throw new TezException("Failed to get response from YARN Timeline: url: " + url);
}
- return response.getEntity(JSONObject.class);
- } catch (ClientHandlerException e) {
+ String json = response.readEntity(String.class);
+ return new JSONObject(json);
+ } catch (Exception e) {
throw new TezException("Error processing response from YARN Timeline. URL=" + url, e);
- } catch (UniformInterfaceException e) {
- throw new TezException("Error accessing content from YARN Timeline - unexpected response. "
- + "URL=" + url, e);
- } catch (IllegalArgumentException e) {
- throw new TezException("Error accessing content from YARN Timeline - invalid url. URL=" + url,
- e);
}
}
private Client getHttpClient() {
if (httpClient == null) {
- ClientConfig config = new DefaultClientConfig(JSONRootElementProvider.App.class);
- HttpURLConnectionFactory urlFactory = new PseudoAuthenticatedURLConnectionFactory();
Review Comment:
I haven't checked this one in detail yet, please let us know @Aggarwal-Raghav if you're fine with @maheshrajus's analysis, so we can maybe move forward
##########
tez-plugins/tez-history-parser/pom.xml:
##########
@@ -145,8 +145,19 @@
<scope>test</scope>
</dependency>
<dependency>
- <groupId>com.sun.jersey</groupId>
- <artifactId>jersey-json</artifactId>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
Review Comment:
some modules introduce `junit-jupiter-api` only, some modules introduce both `junit-jupiter-api` and `junit-vintage-engine`, can you explain the reason for this difference for clarity's sake? ideally, I would expect the same pattern amongst different modules of Tez project (given it's not that huge)
##########
tez-api/src/main/java/org/apache/tez/dag/api/client/TimelineReaderFactory.java:
##########
@@ -203,42 +182,6 @@ private static Authenticator getTokenAuthenticator() throws TezException {
return ReflectionUtils.createClazzInstance(authenticatorClazzName);
}
- private static class TokenAuthenticatedURLConnectionFactory implements HttpURLConnectionFactory {
Review Comment:
I haven't checked this one in detail yet, please let us know @Aggarwal-Raghav if you're fine with @maheshrajus's analysis, so we can maybe move forward
--
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: issues-unsubscribe@tez.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org