Prevent ElasticSearchAliasInit jobs overlapping#1056
Conversation
| use Illuminate\Queue\Middleware\WithoutOverlapping; | ||
| use Illuminate\Support\Facades\Log; | ||
|
|
||
| class ElasticSearchAliasInit extends Job { |
There was a problem hiding this comment.
We could also add $tries and $backoff class properties to this job so that any failed jobs are retried after a period of time. https://laravel.com/docs/12.x/queues#dealing-with-failed-jobs
There was a problem hiding this comment.
This isn't the worst idea. I think perhaps we could also add this with a follow-up if, when we run it, we see any failure. For now I'd leave it be
dati18
left a comment
dati18
left a comment
There was a problem hiding this comment.
I think it's beneficial to add a test function testMiddlewareConfiguration() in the test file.
Note: Something weird I noticed is that the test file is in /tests/Jobs/CirrusSearch/, but the job file is in /Jobs/ instead of /Jobs/CirrusSearch/, which is confusing for me when I was looking for the test file.
Elasticsearch can only process cluster state changes, such as adding new aliases, sequentially on the master node. Runing multiple instances of this job (e.g. with the `EnsureElasticSearchAliases` command) can overload the Elasticsearch master node causing Jobs to fail. Prevent multiple `ElasticSearchAliasInit` jobs from runing at the same time by using the `WithoutOverlapping` middleware [1] [1]: https://laravel.com/docs/10.x/queues#preventing-job-overlaps Bug: T416158
outdooracorn
force-pushed
the
T416158
branch
from
a9d3da5 to
68f3a55
Compare
I did think about that but decided against it as I would effectively be testing the Laravel framework, which already has a test suite.
That would be an unrelated change to this PR. As it would be a small change, I suggest that you create a follow-up PR to address this (and put a Phab task in ready for review). |
| public function middleware(): array { | ||
| return [ | ||
| // Only allow one job per ES host to run at a time to avoid DoSing the ES cluster with alias updates | ||
| new WithoutOverlapping("elasticsearch-alias-init-{$this->esHost}"), |
There was a problem hiding this comment.
praise: nice, and good to do it per cluster rather than globally
dati18
dismissed
their stale review
The test for middleware wiring is not worth the effort and could potentially bite back in the future or add more work.
3 participants
Elasticsearch can only process cluster state changes (e.g. adding new aliases) sequentially on the master node. Running multiple instances of this job (e.g. with the
EnsureElasticSearchAliasescommand) can overload the Elasticsearch master node causing Jobs to fail.Prevent multiple
ElasticSearchAliasInitjobs from running at the same time for each Elasticsearch cluster by using theWithoutOverlappingmiddleware 1Bug: T416158