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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
