flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] zhijiangW commented on a change in pull request #8133: [FLINK-12146][network] Remove unregister task from NetworkEnvironment to simplify the interface of ShuffleService
Date Wed, 17 Apr 2019 07:39:20 GMT
zhijiangW commented on a change in pull request #8133: [FLINK-12146][network] Remove unregister
task from NetworkEnvironment to simplify the interface of ShuffleService
URL: https://github.com/apache/flink/pull/8133#discussion_r276114525
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
 ##########
 @@ -1458,26 +1456,15 @@ private static AbstractInvokable loadAndInstantiateInvokable(
 	private static class TaskCanceler implements Runnable {
 
 		private final Logger logger;
+		private final Task task;
 		private final AbstractInvokable invokable;
 		private final Thread executer;
-		private final String taskName;
-		private final ResultPartition[] producedPartitions;
-		private final InputGate[] inputGates;
-
-		public TaskCanceler(
-				Logger logger,
-				AbstractInvokable invokable,
-				Thread executer,
-				String taskName,
-				ResultPartition[] producedPartitions,
-				InputGate[] inputGates) {
 
+		TaskCanceler(Logger logger, Task task, AbstractInvokable invokable, Thread executer) {
 
 Review comment:
   The motivation is for reusing the `closeOrFailNetworkResources` and avoid static method
and pass the arrays of `ResultPartition` and `InputGate` explicitly.  The previous `AbstractInvokable`
could also be replaced and got from new `Task` parameter.
   
   I think the previous introduced `AbstractInvokable` here is also not a good way considering
exposing more public methods besides `AbstractInvokable#cancel()`. Comparing with `Task` parameter,
we might add more public methods to do so. I agree with reverting this change to pass the
previous specific three parameters here even though the `closeOrFailNetworkResources` might
seem ugly. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message