Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/5105#discussion_r156928873
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/LocalBufferPoolDestroyTest.java
---
@@ -104,11 +104,10 @@ public void testDestroyWhileBlockingRequest() throws
Exception {
* @return Flag indicating whether the Thread is in a blocking buffer
* request or not
*/
- private boolean isInBlockingBufferRequest(StackTraceElement[]
stackTrace) {
+ public static boolean isInBlockingBufferRequest(StackTraceElement[]
stackTrace) {
if (stackTrace.length >= 3) {
return stackTrace[0].getMethodName().equals("wait") &&
-
stackTrace[1].getMethodName().equals("requestBuffer") &&
-
stackTrace[2].getMethodName().equals("requestBufferBlocking");
+
stackTrace[1].getClassName().equals(LocalBufferPool.class.getName());
--- End diff --
I do not like this test in a first place, but I couldn't come up with a
better solution. This change make it at least less implementation dependent.
The same logic as in my "crusade" against mockito. If you put such specific
condition as it was before (blocking on `requestBuffer`) in one more test, you
have to manually fix it in one more place during refactoring/adding features
etc, which drastically increases the cost of maintaining the project and just
doesn't scale up with the project size :(
On the other hand you can argue that if after a refactor, we add more
waiting conditions (if `LocalBufferPool` can block in two different places
depending on some internal condition), broader check like this is might also be
the better choice/condition.
---
