dmvk commented on a change in pull request #18689: URL: https://github.com/apache/flink/pull/18689#discussion_r807972099
########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecution.java ########## @@ -0,0 +1,111 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.accumulators.StringifiedAccumulatorResult; +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.AccessExecution; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.executiongraph.IOMetrics; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +import javax.annotation.Nullable; + +import java.util.Optional; + +/** Mock of an {@link AccessExecution}. */ +public class TestAccessExecution implements AccessExecution { + + private final ExecutionAttemptID executionAttemptID; + private final ExecutionState state; + private final ErrorInfo failureInfo; + private final TaskManagerLocation taskManagerLocation; + + public TestAccessExecution( + ExecutionAttemptID executionAttemptID, + @Nullable ExecutionState state, + @Nullable ErrorInfo failureInfo, + @Nullable TaskManagerLocation taskManagerLocation) { + this.executionAttemptID = executionAttemptID; + this.state = state; + this.failureInfo = failureInfo; + this.taskManagerLocation = taskManagerLocation; + } + + @Override + public ExecutionAttemptID getAttemptId() { + return executionAttemptID; + } + + @Override + public TaskManagerLocation getAssignedResourceLocation() { + if (taskManagerLocation == null) { + throw new UnsupportedOperationException( + "getAssignedResourceLocation should not be called."); + } + return taskManagerLocation; + } + + @Override + public Optional<ErrorInfo> getFailureInfo() { + return Optional.ofNullable(failureInfo); + } + + // -- unsupported methods Review comment: remove ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecution.java ########## @@ -0,0 +1,111 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.accumulators.StringifiedAccumulatorResult; +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.AccessExecution; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.executiongraph.IOMetrics; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +import javax.annotation.Nullable; + +import java.util.Optional; + +/** Mock of an {@link AccessExecution}. */ +public class TestAccessExecution implements AccessExecution { Review comment: 👍 This is headed in a right direction, let's align in with other test classes - `TestAccessExecution` -> `TestingAccessExecution` - Class should have a private constructor (can be only constructed using builder) - Move `TestAccessExecutionBuilder` into this class -> `TestingAccessExecution.Builder` - It should have a private constructor + static factory `TestingAccessExecution.newBuilder()` ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecutionBuilder.java ########## @@ -0,0 +1,58 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.taskmanager.LocalTaskManagerLocation; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +/** Builder for a {@link TestAccessExecution}. */ +public class TestAccessExecutionBuilder { + private ExecutionState state = null; + private ErrorInfo failureInfo = null; Review comment: ```suggestion @Nulllable private ExecutionState state = null; @Nulllable private ErrorInfo failureInfo = null; ``` ########## File path: flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/Canceling.java ########## @@ -55,18 +66,7 @@ public void cancel() { } @Override - public void handleGlobalFailure(Throwable cause) { - getLogger() - .debug( - "Ignored global failure because we are already canceling the job {}.", - getJobId(), - cause); - } - - @Override - boolean updateTaskExecutionState(TaskExecutionStateTransition taskExecutionStateTransition) { - return getExecutionGraph().updateState(taskExecutionStateTransition); - } + void onFailure(Throwable failure) {} Review comment: ```suggestion void onFailure(Throwable failure) { // Why do we ignore failures in this state? } ``` applies to multiple places ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecution.java ########## @@ -0,0 +1,111 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.accumulators.StringifiedAccumulatorResult; +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.AccessExecution; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.executiongraph.IOMetrics; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +import javax.annotation.Nullable; + +import java.util.Optional; + +/** Mock of an {@link AccessExecution}. */ +public class TestAccessExecution implements AccessExecution { + + private final ExecutionAttemptID executionAttemptID; + private final ExecutionState state; + private final ErrorInfo failureInfo; + private final TaskManagerLocation taskManagerLocation; + + public TestAccessExecution( + ExecutionAttemptID executionAttemptID, + @Nullable ExecutionState state, + @Nullable ErrorInfo failureInfo, + @Nullable TaskManagerLocation taskManagerLocation) { Review comment: this seems to be always set in the builder, why is it marked as nullable? ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecutionBuilder.java ########## @@ -0,0 +1,58 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.taskmanager.LocalTaskManagerLocation; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +/** Builder for a {@link TestAccessExecution}. */ +public class TestAccessExecutionBuilder { + private ExecutionState state = null; + private ErrorInfo failureInfo = null; + private ExecutionAttemptID attemptId = new ExecutionAttemptID(); + private TaskManagerLocation taskManagerLocation = new LocalTaskManagerLocation(); + + public TestAccessExecutionBuilder withAttemptId(ExecutionAttemptID attemptId) { + this.attemptId = attemptId; + return this; + } + + public TestAccessExecutionBuilder withExecutionState(ExecutionState state) { + this.state = state; + return this; + } + + public TestAccessExecutionBuilder withErrorInfo(ErrorInfo failureInfo) { + this.failureInfo = failureInfo; + return this; + } + + public TestAccessExecutionBuilder withTaskManagerLocation( + TaskManagerLocation taskManagerLocation) { + this.taskManagerLocation = taskManagerLocation; + return this; + } + + public TestAccessExecution create() { Review comment: ```suggestion public TestAccessExecution build() { ``` ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecution.java ########## @@ -0,0 +1,111 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.accumulators.StringifiedAccumulatorResult; +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.AccessExecution; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.executiongraph.IOMetrics; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +import javax.annotation.Nullable; + +import java.util.Optional; + +/** Mock of an {@link AccessExecution}. */ +public class TestAccessExecution implements AccessExecution { + + private final ExecutionAttemptID executionAttemptID; + private final ExecutionState state; + private final ErrorInfo failureInfo; + private final TaskManagerLocation taskManagerLocation; Review comment: mark nullable fields ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/ExceptionHistoryEntryTest.java ########## @@ -80,11 +75,13 @@ public void testNullExecution() { @Test(expected = NullPointerException.class) public void testNullTaskName() { ExceptionHistoryEntry.create( - new TestAccessExecution( - new ExecutionAttemptID(), - new Exception("Expected failure"), - System.currentTimeMillis(), - new LocalTaskManagerLocation()), + new TestAccessExecutionBuilder() + .withErrorInfo( + new ErrorInfo( + new Exception("Expected failure"), + System.currentTimeMillis())) + .withTaskManagerLocation(new LocalTaskManagerLocation()) Review comment: isn't this the default of the builder? (applies to multiple places) ########## File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/exceptionhistory/TestAccessExecutionBuilder.java ########## @@ -0,0 +1,58 @@ +/* + * 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.scheduler.exceptionhistory; + +import org.apache.flink.runtime.execution.ExecutionState; +import org.apache.flink.runtime.executiongraph.ErrorInfo; +import org.apache.flink.runtime.executiongraph.ExecutionAttemptID; +import org.apache.flink.runtime.taskmanager.LocalTaskManagerLocation; +import org.apache.flink.runtime.taskmanager.TaskManagerLocation; + +/** Builder for a {@link TestAccessExecution}. */ +public class TestAccessExecutionBuilder { + private ExecutionState state = null; + private ErrorInfo failureInfo = null; + private ExecutionAttemptID attemptId = new ExecutionAttemptID(); + private TaskManagerLocation taskManagerLocation = new LocalTaskManagerLocation(); + + public TestAccessExecutionBuilder withAttemptId(ExecutionAttemptID attemptId) { Review comment: Other builders are mostly using the `set...` prefix, it would be great to stick to that -- 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]
