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

Don't close subscriber if channel is recoverable. #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pribor
Copy link

@pribor pribor commented Jul 13, 2016

I made some changes to make the auto recovery work.

  • Ignore a channel shutdown event if it is a recoverable channel that wasn't shut down by an application.
  • Try to close the channel in handleCancel method.
  • QueuePublisher registers shutdown listeners to QueueSubscription.
  • Simplify QueueSubscription termination.

@pribor pribor mentioned this pull request Jul 14, 2016
@mkiedys
Copy link
Contributor

mkiedys commented Jul 17, 2016

LGTM

@dskrvk
Copy link

dskrvk commented Aug 27, 2016

What's the status here?

@mkiedys
Copy link
Contributor

mkiedys commented Aug 29, 2016

Hi @pribor! Can you rebase?

@mkiedys mkiedys self-assigned this Aug 29, 2016
@pribor
Copy link
Author

pribor commented Sep 7, 2016

Done

@DGolubets
Copy link

DGolubets commented Sep 23, 2016

I tried it: killed rabbit, started it again, but my streams didn't resume..

@mkiedys mkiedys force-pushed the master branch 3 times, most recently from 9d5fb44 to 531d688 Compare November 18, 2016 12:12
@kerzok
Copy link

kerzok commented Nov 24, 2016

Is there any chance that this PR will be accepted?
I tried to run it and seems to streams resuming works correctly.

@marius-carp
Copy link

It's been a while since this PR is opened, what is the reason for not merging it?

@duketon
Copy link

duketon commented Feb 13, 2017

Bump, with the above question.

@mkiedys
Copy link
Contributor

mkiedys commented Feb 13, 2017

Can someone confirm if this works?

@marius-carp
Copy link

I'm using it in staging environment and it seems to be working.

@djamelz
Copy link

djamelz commented Mar 1, 2017

Hi guys, it works very well ! Is it possible to merge it ? (BTW more than the branch conflict, I was forced to refactor a little bit the code to build it in scala 2.12)

@muub
Copy link

muub commented May 16, 2017

Hey folks, We are actively using this fix in production. We had to pin to it in order to continue using this library. We would very much appreciate having this PR merged. If we can help in any way please let me know.

@guizmaii
Copy link
Contributor

What's the pb with this PR ? @mkiedys ? What should we do to make you accept it ?

@yspotts
Copy link

yspotts commented Jun 6, 2017

Just want to add our voice to the mix. We are having a issue in production caused by this bug. We are going to deploy our own version based on this PR. Would love to see this PR get merged in the near future! Thanks for a great library!

@mkiedys
Copy link
Contributor

mkiedys commented Jun 6, 2017

The reason why this stayed open so long are:

  • conflicting reviews suggesting this isn't working
  • no tests
  • no clear story how to replicate and verify if it works

I'm looking into this right now.

@dskrvk
Copy link

dskrvk commented Jun 20, 2018

What's the relationship to #64?

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.

None yet