Fix EsAbortPolicy to conform to API #29075
Conversation
The rejected execution handler API says that rejectedExecution(Runnable, ThreadPoolExecutor) throws a RejectedExecutionException if the task must be rejected due to capacity on the executor. We do throw something that smells like a RejectedExecutionException (it is named EsRejectedExecutionException) yet we violate the API because EsRejectedExecutionException is not a RejectedExecutionException. This has caused problems before where we try to catch RejectedExecution when invoking rejectedExecution but this causes EsRejectedExecutionException to go uncaught. This commit addresses this by modifying EsRejectedExecutionException to extend RejectedExecutionException. Addtionally, we fix an issue with how causes are unwrapped. Without this change, since EsRejectedExecutionException is no longer an ElasticsearchException, the root cause of a rejection would be reported as a remote_transport_exception instead of as an es_rejected_execution_exception.
jasontedor
added
review
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
|
retest this please |
That was the plan but after I started preparing that I realized it is a little hairy. I pushed it here and will change the version after I backport. Would you please take a look at the bridge that I pushed here? |
I think that this change should be made in a separate change so I have pulled it out of this one. |
|
@bleskes Can you take another look? |
| } else if (throwable instanceof EsRejectedExecutionException) { | ||
| // TODO: remove the if branch when master is bumped to 8.0.0 | ||
| assert Version.CURRENT.major < 8; | ||
| if (version.before(Version.V_7_0_0_alpha1)) { |
There was a problem hiding this comment.
It seems like you're not planning to back port this any more. Can you push a comment in the 6.x branch to note that if you change the wire level there, you need to look at 7.x code too? I don't think we will so a comment seems enough (a test is tricky)
There was a problem hiding this comment.
I am planning to backport. I do it this way so BWC tests can pass on this branch. Then I backport and change the version. Then I remove this code from master.
A test is very tricky indeed. I thought about a qa test in 6.x only (with a bulk thread pool with queue 0 to force a rejection). I am not sure it’s worth it. I did test it manually like this.
There was a problem hiding this comment.
Agreed that a test is an overkill. At least any test I can think of.
The rejected execution handler API says that rejectedExecution(Runnable, ThreadPoolExecutor) throws a RejectedExecutionException if the task must be rejected due to capacity on the executor. We do throw something that smells like a RejectedExecutionException (it is named EsRejectedExecutionException) yet we violate the API because EsRejectedExecutionException is not a RejectedExecutionException. This has caused problems before where we try to catch RejectedExecution when invoking rejectedExecution but this causes EsRejectedExecutionException to go uncaught. This commit addresses this by modifying EsRejectedExecutionException to extend RejectedExecutionException.
* master: (476 commits) Fix compilation errors in ML integration tests Small code cleanups and refactorings in persistent tasks (elastic#29109) Update allocation awareness docs (elastic#29116) Configure error file for archive packages (elastic#29129) Configure heap dump path for archive packages (elastic#29130) Client: Add missing test getMinGenerationForSeqNo should acquire read lock (elastic#29126) Backport - Do not renew sync-id PR to 5.6 and 6.3 Client: Wrap SSLHandshakeException in sync calls Fix creating keystore when upgrading (elastic#29121) Align thread pool info to thread pool configuration (elastic#29123) TEST: Adjust translog size assumption in new engine Docs: HighLevelRestClient#multiGet (elastic#29095) Client: Wrap synchronous exceptions (elastic#28919) REST: Clear Indices Cache API simplify param parsing (elastic#29111) Fix typo in ExceptionSerializationTests Remove BWC layer for rejected execution exception Fix EsAbortPolicy to conform to API (elastic#29075) [DOCS] Removed prerelease footnote from upgrade table. Docs: Support triple quotes (elastic#28915) ...
The rejected execution handler API says that rejectedExecution(Runnable, ThreadPoolExecutor) throws a RejectedExecutionException if the task must be rejected due to capacity on the executor. We do throw something that smells like a RejectedExecutionException (it is named EsRejectedExecutionException) yet we violate the API because EsRejectedExecutionException is not a RejectedExecutionException. This has caused problems before where we try to catch RejectedExecution when invoking rejectedExecution but this causes EsRejectedExecutionException to go uncaught. This commit addresses this by modifying EsRejectedExecutionException to extend RejectedExecutionException. Addtionally, we fix an issue with how causes are unwrapped. Without this change, since EsRejectedExecutionException is no longer an ElasticsearchException, the root cause of a rejection would be reported as a remote_transport_exception instead of as an es_rejected_execution_exception.
Closes #19508