YARN-11786. Upgrade hadoop-yarn-server-timelineservice-hbase-tests to Support Trunk Compilation and Remove compatible hadoop version. #7453
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,32 +22,29 @@ | |
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| import javax.ws.rs.client.Client; | ||
| import javax.ws.rs.client.ClientBuilder; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.HttpURLConnection; | ||
| import java.net.URI; | ||
| import java.util.List; | ||
|
|
||
| import javax.ws.rs.core.GenericType; | ||
| import javax.ws.rs.core.MediaType; | ||
| import javax.ws.rs.core.Response; | ||
|
|
||
| import org.apache.hadoop.classification.VisibleForTesting; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.hbase.HBaseTestingUtility; | ||
| import org.apache.hadoop.yarn.api.records.timelineservice.FlowActivityEntity; | ||
| import org.apache.hadoop.yarn.api.records.timelineservice.reader.TimelineEntityReader; | ||
| import org.apache.hadoop.yarn.conf.YarnConfiguration; | ||
| import org.apache.hadoop.yarn.server.timelineservice.storage.DataGeneratorForTest; | ||
| import org.glassfish.jersey.client.ClientConfig; | ||
| import org.glassfish.jersey.client.HttpUrlConnectorProvider; | ||
| import org.junit.Assert; | ||
|
|
||
| /** | ||
| * Test Base for TimelineReaderServer HBase tests. | ||
| */ | ||
|
|
@@ -109,19 +106,25 @@ protected void addFilters(Configuration conf) { | |
| } | ||
|
|
||
| protected Client createClient() { | ||
| final ClientConfig cc = new ClientConfig(); | ||
| cc.connectorProvider(getHttpURLConnectionFactory()); | ||
| return ClientBuilder.newClient(cc) | ||
| .register(TimelineEntityReader.class) | ||
|
There was a problem hiding this comment. I'm not familiar with differences in the new Jersey version, but it seems like the prior code was much more driven by introspection and annotations instead of needing to register a lot of custom readers. Is there any way to make it more like the prior code? I'm concerned about risk of backward-incompatible changes in the manual approach and potential error-prone evolution of the code if fields are changed in the future. There was a problem hiding this comment. This is a good question. I believe we need to update some parts of the code to align with Jersey 2's standards, as certain parts of the code are not well-supported by the latest standards. I first became aware of the issue with Jersey during the release of Hadoop 3.4.0, when aajisaka mentioned that, with Hadoop supporting JDK 11, there was an area that needed optimization — Jersey support. For more details, you can refer to HADOOP-15984 (updating Jersey from 1.19 to 2.x). Jersey is a framework that supports RESTful WebService. When upgrading from version 1.x to 2.x, compatibility issues arise because these two versions are completely incompatible, with even basic methods being different. To address this, we define custom readers because Jersey 2.x only supports automatic JSON conversion for basic data types and does not support automatic conversion for more complex types. More details can be found in the official documentation. While writing these readers, I used After upgrading to JUnit 5, we will configure a new JDK 17 CI pipeline. Following that, I will begin the deeper adaptation and upgrade of Jersey 2. There was a problem hiding this comment. @slfan1989 , thank you for the reply. This is helpful information. It sounds like some of this is just unavoidable with this Jersey upgrade. The biggest thing I'm concerned about is the potential to break YARN's user-facing APIs. Please correct me if I'm wrong, but I think we want to maintain full API compatibility in the 3.5.0 release. If so, how can we be confident about this? Maybe we need to spin up some kind of test of a 3.4 client against a 3.5 server? There was a problem hiding this comment. @cnauroth Thank you for your message. We need to ensure the compatibility of the interfaces. For the YARN REST API, most interfaces have no compatibility issues. Even after upgrading to Jersey 2, the changes to the server's WebService are minimal, usually just adding readers and writers. While there are still a few interfaces that need re-validation, I have a general understanding of the areas that need to be checked. We can work together to complete this task, which must be done before the release of 3.5.0. |
||
| .register(TimelineEntitySetReader.class) | ||
| .register(TimelineEntityListReader.class) | ||
| .register(FlowActivityEntityReader.class) | ||
| .register(FlowRunEntityReader.class) | ||
| .register(FlowActivityEntitySetReader.class) | ||
| .register(FlowActivityEntityListReader.class) | ||
| .register(FlowRunEntitySetReader.class); | ||
| } | ||
|
|
||
| protected Response getResponse(Client client, URI uri) | ||
| throws Exception { | ||
| Response resp = | ||
| client.target(uri).request(MediaType.APPLICATION_JSON).get(); | ||
| if (resp == null || resp.getStatusInfo() | ||
| .getStatusCode() != HttpURLConnection.HTTP_OK) { | ||
| String msg = ""; | ||
| if (resp != null) { | ||
| msg = String.valueOf(resp.getStatusInfo().getStatusCode()); | ||
|
|
@@ -132,39 +135,38 @@ protected ClientResponse getResponse(Client client, URI uri) | |
| return resp; | ||
| } | ||
|
|
||
| protected void verifyHttpResponse(Client client, URI uri, Response.Status status) { | ||
| Response resp = client.target(uri).request(MediaType.APPLICATION_JSON).get(); | ||
| assertNotNull(resp); | ||
| assertTrue("Response from server should have been " + status, | ||
| resp.getStatusInfo().getStatusCode() == status.getStatusCode()); | ||
| System.out.println("Response is: " + resp.readEntity(String.class)); | ||
| } | ||
|
|
||
| protected List<FlowActivityEntity> verifyFlowEntites(Client client, URI uri, | ||
| int noOfEntities) throws Exception { | ||
| Response resp = getResponse(client, uri); | ||
| List<FlowActivityEntity> entities = | ||
| resp.readEntity(new GenericType<List<FlowActivityEntity>>() { | ||
| }); | ||
| assertNotNull(entities); | ||
| assertEquals(noOfEntities, entities.size()); | ||
| return entities; | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| protected HttpUrlConnectorProvider getHttpURLConnectionFactory() { | ||
| return new HttpUrlConnectorProvider().connectionFactory( | ||
| url -> { | ||
| HttpURLConnection conn; | ||
| try { | ||
| HttpURLConnection.setFollowRedirects(false); | ||
|
There was a problem hiding this comment. Why was it necessary to disable following redirects? This change is applied globally for the whole JVM. I wonder if it could have unintended side effects for other areas that expect to follow redirects (which is the default behavior). There was a problem hiding this comment. That's a good question, and you're right, it's unnecessary. I can remove the code |
||
| conn = (HttpURLConnection) url.openConnection(); | ||
| } catch (Exception e) { | ||
| throw new IOException(e); | ||
| } | ||
| return conn; | ||
| }); | ||
| } | ||
|
|
||
| protected static HBaseTestingUtility getHBaseTestingUtility() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this potentially a backward-incompatible change? Could we start returning "1970" where previously the value was null/missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing this PR! We encountered a NullPointerException during unit testing. For the creation time, we need to set a default value, and I chose Instant.EPOCH over Instant.Min. In order to explain the issue clearly, we need to mention Jersey. I will provide a detailed explanation in response to your third question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rolled back this part of the code.