flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pnowojski <...@git.apache.org>
Subject [GitHub] flink pull request #5105: [FLINK-8178][network] Introduce not threadsafe wri...
Date Fri, 15 Dec 2017 13:15:15 GMT
Github user pnowojski commented on a diff in the pull request:

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


View raw message