a config initialized for queue timeout

[hkulekci]

#229 I don't like this solution. But I created a PR to see what we can do and discuss on it.

[matchish]

Why don't you like it? I don't like the cause of the issue. I'm in general would like send many independent jobs to queue so we don't have long living jobs. But for some reason I was not satisfied with the solution #78

[hkulekci]

First, I feel like this configuration should be inside the queue itself. But we are putting a separate configuration for this queue.

Second, there is no $timeout property inside the class. Maybe we need to put a $timeout property inside the . For now, the Editor is giving a warning at line 54. Also another solution, we can use the class itself as configuration for queue class. And the user can change whatever he want for the queue with their custom class.

But also, for some reason, RDMS can be very slow, and the time out can be exceeded. Paralel import can not be a solution somehow when the data provider is really slow.

I could not check the details of #78, but maybe I can check over the weekend.

[codecov-commenter]

Codecov Report

Base: 95.80% // Head: 96.04% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (494d31c) compared to base (e545cde).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #230      +/-   ##
============================================
+ Coverage     95.80%   96.04%   +0.24%     
- Complexity      184      188       +4     
============================================
  Files            34       36       +2     
  Lines           524      632     +108     
============================================
+ Hits            502      607     +105     
- Misses           22       25       +3     

Impacted Files	Coverage Δ
src/Jobs/Import.php	100.00% <ø> (ø)
src/Jobs/QueueableJob.php	100.00% <ø> (ø)
src/Console/Commands/ImportCommand.php	100.00% <100.00%> (ø)
src/MixedSearch.php	100.00% <0.00%> (ø)
src/ElasticSearch/Index.php	100.00% <0.00%> (ø)
src/ElasticSearch/Params/Bulk.php	100.00% <0.00%> (ø)
src/Database/Scopes/ChunkScope.php	0.00% <0.00%> (ø)
src/Jobs/Stages/PullFromSource.php	100.00% <0.00%> (ø)
src/ElasticSearch/Params/Search.php	100.00% <0.00%> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[matchish]

True but as we don't have other user cases to modify queue properties I think this solution acceptable. Simple and works. Next time we can think about more robust solution. There is a risk for backward compatibility if we will introduce better solution later. But I think that's ok.
Slow data providers for sure can be an issue but I would prefer to have chunks more small then increase timeouts. If default timeout expired than probably there are some infrastructure issue and better to reschedule the job than increase timeout. I mean default timeout should be enough if chunk size chosen according for expected data provider speed, if it's slower, than it could be even alert for monitoring infrastructure, that something going wrong.

[hkulekci]

So, we just need to improve the tests and merge this PR. Let me check the tests again for this PR. I will ping you again.

[hkulekci]

While testing this, I tried to understand how this part worked again. And, I am confused a little bit. We are chaining the jobs in here, right?

In here, on line 50, we are creating a job as Import and we chain this job to QueueableJob on line 54. And we are setting the timeout to QueueableJob only. I think we need to set timeout to Import job, too, right? Setting timeout for the QueueableJob won't be enough as I understand.

[hkulekci]

For the testing side, I could not find how we can test this part inside the Feature tests. I created a DelayedImportSourceFactory and DelayedImportSource with sleep(5) function and I set the timeout as 2 seconds but it is not working with sync connection as expected.

[matchish]

True I think we have to set it to both jobs

[matchish]

For testing instead of sleep I believe it's better to use bus fake and check jobs have proper timeouts https://laravel.com/docs/9.x/mocking#bus-fake

[hkulekci]

Let me check this. Thanks. ✌️

[hkulekci]

Could you check the Buss::fake() part? I just fake the queues one by one and check whether the timeout field is set correctly or not. @matchish

[matchish]

If you change assert to
Bus::assertDispatched(function (Import $job) {
return $job->timeout === 7;
});
and it's fail than test is good

[hkulekci]

It is failing when I do it.

PHPUnit 9.4.4 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.1
Configuration: /Volumes/webroot/github/laravel-scout-elasticsearch/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

F                                                                   1 / 1 (100%)

Time: 00:00.496, Memory: 36.00 MB

There was 1 failure:

1) Tests\Feature\ImportCommandTest::test_chained_queue_timeout_configuration
The expected [Matchish\ScoutElasticSearch\Jobs\Import] job was not dispatched.
Failed asserting that false is true.

/Volumes/webroot/github/laravel-scout-elasticsearch/vendor/laravel/framework/src/Illuminate/Support/Testing/Fakes/BusFake.php:90
/Volumes/webroot/github/laravel-scout-elasticsearch/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:261
/Volumes/webroot/github/laravel-scout-elasticsearch/tests/Feature/ImportCommandTest.php:223

FAILURES!
Tests: 1, Assertions: 3, Failures: 1.

Do I need to do a negative tests, too?

[matchish]

No) It's just red green approach for writing tests.first you write a test that fails then fix it to pass. Of course it's better to change code to have red test but here I don't see difference we can change test to have failed test as well. To summarize no I don't see a reason to have negative tests)

[hkulekci]

I think I could not understand the main topic of what is preventing merging this PR to the repository.

[matchish]

I believe it's ready to merge. just wanted to have feedback from @amcsi

[matchish]

Let's wait one week more if you don't need it asap

[amcsi]

Thanks, I have left a review. No need to hurry with this for me.

[hkulekci]

Failed tests are related to this : php-http/discovery#217

[hkulekci]

We need to wait for php-http/discovery issue. I found this PR and saying that we fixed the bug.

php-http/discovery#219

But I guess, it is not working for us.

[amcsi]

Approved from my side 👍 apart from the CI failures

[hkulekci]

I created an issue with elasticsearch client and waiting for the merge. After this merge, we can rerun the tests to see green. Then, I guess we can merge this PR.

Thanks for excellent review @amcsi

[hkulekci]

So, Elasticsearch merged the PR to the main branch. I am waiting for the composer updates and than we can rerun the tests again.

[hkulekci]

So, we are good. Tests passed in the end. @matchish