Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(2228): added transaction free deleteAndRelease() Method #2229

Merged
merged 7 commits into from May 2, 2021

Conversation

sodoardi
Copy link
Contributor

@sodoardi sodoardi commented Apr 6, 2021

fixes: #2228

@divine
Copy link
Contributor

divine commented Apr 6, 2021

Duplicate #2180

@sodoardi
Copy link
Contributor Author

sodoardi commented Apr 7, 2021

Wow.. didn't see that, looked everywhere to find a solution 🤦‍♂️
How can we get one of the fixes published? Looks like the other fix is stuck since 3 months now

alliuqemal
alliuqemal previously approved these changes Apr 21, 2021
@sodoardi
Copy link
Contributor Author

@divine any updates on how best to proceed? Close this PR and wait for the other one to be released or how do we plan on getting this bug fixed?

@divine
Copy link
Contributor

divine commented Apr 21, 2021

@divine any updates on how best to proceed? Close this PR and wait for the other one to be released or how do we plan on getting this bug fixed?

Hello,

I will talk with @Smolevich today and let you know what we've decided.

Thanks!

@Smolevich Smolevich self-assigned this Apr 22, 2021
tests/QueueTest.php Show resolved Hide resolved
@sodoardi
Copy link
Contributor Author

I'll take a look this weekend

@divine
Copy link
Contributor

divine commented May 1, 2021

I'll take a look this weekend

Any update?

Thanks!

@sodoardi
Copy link
Contributor Author

sodoardi commented May 2, 2021

I'll take a look this weekend

Any update?

Thanks!

@divine at least grant me the whole weekend 😁
@Smolevich I added a test calling directly to the Queue::deleteAndRelease() method, I hope thats how you meant it!

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2021

Codecov Report

Merging #2229 (0b8f99b) into master (753cfde) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2229      +/-   ##
============================================
+ Coverage     86.88%   86.91%   +0.02%     
- Complexity      664      665       +1     
============================================
  Files            33       33              
  Lines          1556     1559       +3     
============================================
+ Hits           1352     1355       +3     
  Misses          204      204              
Impacted Files Coverage Δ Complexity Δ
src/Queue/MongoQueue.php 100.00% <100.00%> (ø) 11.00 <1.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 753cfde...0b8f99b. Read the comment docs.

@divine
Copy link
Contributor

divine commented May 2, 2021

@divine at least grant me the whole weekend 😁

Sorry but this is an open-source, contributors vanishing sometimes without any reply... and it was urgent for you too so no waiting whole weekend 🤣

@Smolevich I added a test calling directly to the Queue::deleteAndRelease() method, I hope thats how you meant it!

I think he meant this part:

$this->assertEquals(0, $result['reserved']);
$this->assertNull($result['reserved_at']);
$this->assertEquals(
        Carbon::now()->addRealSeconds($delay)->getTimestamp(),
        $result['available_at']
);
$this->assertEquals(Carbon::now()->getTimestamp(), $result['created_at']);
$this->assertEquals($job->getRawBody(), $result['payload']);

Thanks!

@sodoardi
Copy link
Contributor Author

sodoardi commented May 2, 2021

@divine oh no, I hope nothing bad happens to them. Should I be worried about my safety? 🤣
I did some restructuring on the tests, hope that works out! I won't be able to check back until this evening though (european time)

@Smolevich Smolevich requested review from Smolevich and divine May 2, 2021 07:49
Copy link
Contributor

@Smolevich Smolevich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolve merge conflicts

@divine divine requested a review from Smolevich May 2, 2021 08:02
@divine divine removed the Needs work label May 2, 2021
@divine
Copy link
Contributor

divine commented May 2, 2021

@divine oh no, I hope nothing bad happens to them. Should I be worried about my safety? 🤣
I did some restructuring on the tests, hope that works out! I won't be able to check back until this evening though (european time)

Don't worry about your safety, we're just having a fun conversation, so I ruined fun now completely 😄.

I really don't know what happens with them, probably too busy and it just works 😮.

Anyways, thank you for your contribution!

@divine divine merged commit 5d1416d into mongodb:master May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Laravel 8/MongoDB - job->release() leads to Call to a member function beginTransaction() on null error
5 participants