Aggarwal-Raghav commented on code in PR #474:
URL: https://github.com/apache/tez/pull/474#discussion_r3276214338
##########
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:
@maheshrajus , I'm not in favour of removing
`TokenAuthenticatedURLConnectionFactory` and
`PseudoAuthenticatedURLConnectionFactory` class .
They were implementing `HttpURLConnectionFactory` which is jersey 1.x and
going forward they should implement
`HttpUrlConnectorProvider.ConnectionFactory` jersey 2.x
To summarize, in this PR you are creating a client without plugging
`ConnectionFactory`
Can you check this patch on top your PR?
[Timeline.patch](https://github.com/user-attachments/files/28069615/Timeline.patch)
##########
tez-ext-service-tests/pom.xml:
##########
@@ -53,6 +53,11 @@
<artifactId>hadoop-hdfs</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
Review Comment:
Can we move this in parent pom by introducing `<dependencies>` tag and
remove from all the sub-modules?
```
<dependencies>
<dependency>
<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>
<scope>test</scope>
</dependency>
</dependencies>
```
##########
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:
Same comment as in `TimelineReaderFactory` the client created is a vanila
client. I disagee with the statement
> PseudoAuthenticatedURLConnectionFactory removed in Jersey 2.x and for
Pseudo auth strategy for env where delegation token is not supported (hadoop
2.4)
I have 2 questions on this statement:
1. "PseudoAuthenticatedURLConnectionFactory removed in Jersey 2.x" :
`PseudoAuthenticatedURLConnectionFactory` is TEZ internal class, the
implementation should have been updated to the Jersey2.x `ConnectionFactory`
interface, not deleted.
2. "Pseudo auth strategy for env where delegation token is not supported
(hadoop 2.4)" , then why in hadoop-3.4.1 it was still present in TEZ code?
--
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]
