[GitHub] [flink] xintongsong commented on a change in pull request #17496: [FLINK-24562][yarn] YarnResourceManagerDriverTest should not use ContainerStatusPBImpl.newInstance

[GitBox <gi...@apache.org>]

xintongsong commented on a change in pull request #17496:
URL: https://github.com/apache/flink/pull/17496#discussion_r731439124



##########
File path: flink-yarn/src/test/java/org/apache/flink/yarn/TestingContainerStatus.java
##########
@@ -21,16 +21,23 @@
 import org.apache.hadoop.yarn.api.records.ContainerId;
 import org.apache.hadoop.yarn.api.records.ContainerState;
 import org.apache.hadoop.yarn.api.records.ContainerStatus;
-import org.apache.hadoop.yarn.api.records.impl.pb.ContainerStatusPBImpl;
 
 /** A {@link ContainerStatus} implementation for testing. */
-class TestingContainerStatus extends ContainerStatusPBImpl {
+class TestingContainerStatus extends ContainerStatus {
 
     private final ContainerId containerId;
     private final ContainerState containerState;
     private final String diagnostics;
     private final int exitStatus;
 
+    public static ContainerStatus newInstance(
+            ContainerId containerId,
+            ContainerState containerState,
+            String diagnostics,
+            int exitStatus) {
+        return new TestingContainerStatus(containerId, containerState, diagnostics, exitStatus);
+    }

Review comment:
       Why do we need this factory method while we already have a constructor with the exactly same arguments?

##########
File path: flink-yarn/src/test/java/org/apache/flink/yarn/TestingContainerStatus.java
##########
@@ -21,16 +21,23 @@
 import org.apache.hadoop.yarn.api.records.ContainerId;
 import org.apache.hadoop.yarn.api.records.ContainerState;
 import org.apache.hadoop.yarn.api.records.ContainerStatus;
-import org.apache.hadoop.yarn.api.records.impl.pb.ContainerStatusPBImpl;
 
 /** A {@link ContainerStatus} implementation for testing. */
-class TestingContainerStatus extends ContainerStatusPBImpl {
+class TestingContainerStatus extends ContainerStatus {

Review comment:
       I'm not sure about this change.
   
   The reason we extends a concrete implementation rather than the abstract class here is that, the abstract methods need to be implemented for the abstract class could be different in different hadoop versions. Extending a concrete implementation makes sure all abstract methods required by the hadoop version it compiles with are implemented.
   
   This is probably not a problem for `ContainerStatus`, but we have encountered problems in other hadoop classes, which we workaround in this way. It might be nice to align the ways we handle these hadoop classes.




-- 
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@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org