pnowojski commented on a change in pull request #16773: URL: https://github.com/apache/flink/pull/16773#discussion_r688911300
########## File path: flink-tests/src/test/java/org/apache/flink/runtime/operators/lifecycle/command/TestCommandQueue.java ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.runtime.operators.lifecycle.command; + +import org.apache.flink.testutils.junit.SharedObjects; + +import java.io.Serializable; + +/** A queue for {@link TestCommand} executed by operators in a test job. */ +public interface TestCommandQueue extends Serializable { + + void add(TestCommand testCommand, TestCommandTarget target, TestCommandRetention retention); Review comment: What is easier to understand by a quick glance at the code snippets below? ``` dispatch(TestCommand testCommand, TestCommandScope retention, String operatorID)` ``` or ``` dispatchToAllSubtasks(TestCommand testCommand, String operatorID); dispatchToSingleSubtask(TestCommand testCommand, String operatorID); ``` ? The first one is not self explanatory. You need to jump between many places in the code to understand how does it work. What is the price that we would be pay by having two methods instead of one? More methods in the interfaces? Do we really need those interfaces?`SharedTestCommandDispatcher`, `TestCommandDispatcher` and `TestCommandDispatcherImpl` should be also squashed. Again, encapsulating a single field for a class that could be implemented in 60 lines of code (including imports) is not worth adding additional 100 lines of code and having to jump through 3 additional files to understand what the code is doing. The only two remaining interface places affected by dropping `TestCommandScope` would be: ``` public TestJobExecutor sendOperatorCommand( String operatorID, TestCommandScope retention, TestCommand testCommand, TestCommandDispatcher commandQueue) { LOG.debug("sendCommand: {}", testCommand); commandQueue.dispatch(testCommand, retention, operatorID); return this; } public TestJobExecutor sendBroadcastCommand( TestCommandScope retention, TestCommand testCommand, TestCommandDispatcher commandQueue) { LOG.debug("sendCommand: {}", testCommand); commandQueue.broadcast(testCommand, retention); return this; } ``` which also should be inlined with `LOG` moved into the `TestCommandDispatcher`. Almost all of the `TestJobExecutor` could be inlined apart of methods touching `job` and `miniClusterResource`. All in all, those changes that I highlighted above would reduce number of "jumps" in the IDE to understand how this call is working: ``` .sendBroadcastCommand(SINGLE_SUBTASK, FAIL, testJob.commandQueue) ``` from ~6 down to a single jump in: ``` testJob.commandQueue.sendToAllSubtasksOfAllTasks(FAIL); ``` -- 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]
