[GitHub] [flink] XComp commented on a change in pull request #13004: [FLINK-18719][runtime] Define interfaces for new active RMs and implement

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

XComp commented on a change in pull request #13004:
URL: https://github.com/apache/flink/pull/13004#discussion_r462269930



##########
File path: flink-core/src/main/java/org/apache/flink/util/ExceptionUtils.java
##########
@@ -349,6 +349,18 @@ public static void tryRethrowException(@Nullable Exception e) throws Exception {
 		}
 	}
 
+	/**
+	 * Tries to throw the given throwable if not null.
+	 *
+	 * @param t throwable to throw if not null
+	 * @throws Throwable
+	 */
+	public static void tryRethrowThrowable(@Nullable Throwable t) throws Throwable {

Review comment:
       We could combine `tryRethrowThrowable(Throwable)` and `tryRethrowException(Exception)`:
   ```suggestion
   	public static <T extends Throwable> void tryRethrowThrowable(@Nullable T t) throws T {
   	 	if (t != null) {
   			throw t;
   		}
   	}
   ```

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/JvmMetaspaceAndOverhead.java
##########
@@ -49,4 +50,16 @@ public MemorySize getMetaspace() {
 	public MemorySize getOverhead() {
 		return overhead;
 	}
+
+	@Override
+	public boolean equals(Object obj) {

Review comment:
       Providing a customized `equals(Object)` usually means that one should also provide a corresponding `hashCode()` implementation. Is the `hashCode()` method missing on purpose?

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/jobmanager/JobManagerFlinkMemory.java
##########
@@ -50,7 +53,8 @@
 	private final MemorySize jvmHeap;
 	private final MemorySize offHeapMemory;
 
-	JobManagerFlinkMemory(MemorySize jvmHeap, MemorySize offHeapMemory) {
+	@VisibleForTesting

Review comment:
       Why do we use the `@VisibleForTesting` annotation here? It looks like the constructor is also used in the context of production code.

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/jobmanager/JobManagerFlinkMemory.java
##########
@@ -69,4 +73,16 @@ public MemorySize getJvmDirectMemorySize() {
 	public MemorySize getTotalFlinkMemorySize() {
 		return jvmHeap.add(offHeapMemory);
 	}
+
+	@Override
+	public boolean equals(Object obj) {

Review comment:
       Providing a customized `equals(Object)` usually means that one should also provide a corresponding `hashCode()` implementation. Is the `hashCode()` method missing on purpose?

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/CommonProcessMemorySpec.java
##########
@@ -94,4 +96,16 @@ public MemorySize getTotalFlinkMemorySize() {
 	public MemorySize getTotalProcessMemorySize() {
 		return flinkMemory.getTotalFlinkMemorySize().add(getJvmMetaspaceSize()).add(getJvmOverheadSize());
 	}
+
+	@Override
+	public boolean equals(Object obj) {

Review comment:
       Providing a customized `equals(Object)` usually means that one should also provide a corresponding `hashCode()` implementation. Is the `hashCode()` method missing on purpose?

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/util/config/memory/taskmanager/TaskExecutorFlinkMemory.java
##########
@@ -128,4 +130,20 @@ public MemorySize getJvmDirectMemorySize() {
 	public MemorySize getTotalFlinkMemorySize() {
 		return frameworkHeap.add(frameworkOffHeap).add(taskHeap).add(taskOffHeap).add(network).add(managed);
 	}
+
+	@Override
+	public boolean equals(Object obj) {

Review comment:
       Providing a customized `equals(Object)` usually means that one should also provide a corresponding `hashCode()` implementation. Is the `hashCode()` method missing on purpose?




----------------------------------------------------------------
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