Add utility function to check if an executor is direct or not#143
Add utility function to check if an executor is direct or not#143krka wants to merge 1 commit intospotify:masterfrom
Conversation
krka
force-pushed
the
krka/directexecutor
branch
3 times, most recently
from
d92ec07 to
e4f772b
Compare
Don't move futures to different executor if it's a direct executor
krka
force-pushed
the
krka/directexecutor
branch
from
e4f772b to
c569754
Compare
| int maxSetLength) { | ||
| this.address = address; | ||
| this.executor = executor; | ||
| this.isDirectExecutor = executor == null || Utils.isDirectExecutor(executor); |
There was a problem hiding this comment.
Nitpicking here, but given isDirectExecutor really meaning "is a direct executor or null", how about rather naming it something like "wrapWithExecutor"? (I'm terrible at naming so I'm sure an even better name would be available)
| // 1) It is a direct executor, so runnableWithThread.thread will be set to the current thread. | ||
| // We will correctly return true | ||
| // 2) It is not a direct executor and runnableWithThread.thread is still null when we check | ||
| // it. |
There was a problem hiding this comment.
Nitpicking: wrong indentation
|
|
||
| return runnableWithThread.thread == Thread.currentThread(); | ||
| } catch (Exception e) { | ||
| // If the executor throws any exception, it can not be a direct executor |
There was a problem hiding this comment.
Is this as safe assumption? Looking at the Guava DirectExecutorService, it can throw exceptions here, but only if the executor is shutting down, which is probably fine either way. But, would we want to assume other direct executors would behave the same way?
There was a problem hiding this comment.
No, probably not a safe assumption. It depends a bit on what the function is used for. If it's only for optimization purposes and sanity checks it may still be good enough
4 participants
Don't move futures to different executor if it's a direct executor