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

Allow creation of AWS topic on existing queues #66

Closed
wants to merge 2 commits into from

Conversation

pmartelletti
Copy link

If you provide the Queue name (which already exists in SQS), but you don't have a Topic to respond to this queue, it will call the create() function, which will attempt to create the query again (although it already exists). Need to check that in the create function as well (and not only in the publish method).

I know it does not makes sense to call the create method and check if the Queue exists or not, but if not, a refactor would be need to separete que creation of the queue and the topic one, so that on publish method, if the topic is not created, we just called the createTopic method and not the create function.

Does this make sense to any of you guys? Or I'm doing anything wrong?

If you provide the Queue name (which already exists in SQS), but you don't have a Topic to respond to this queue, it will call the `create()` function, which will attempt to create the query again (although it already exists). Need to check that in the create function as well (and not only in the publish method).

I know it does not makes sense to call the `create` method and check if the Queue exists or not, but if not, a refactor would be need to separete que creation of the queue and the topic one, so that on publish method, if the topic is not created, we just called the createTopic method and not the create function.

Does this make sense to any of you guys?
Allow existing Queues to create topic
@scrutinizer-notifier
Copy link

A new inspection was created.

@@ -20,7 +20,7 @@
* @license Apache License, Version 2.0
*/

namespace Uecode\Bundle\QPushBundle\Provider;
namespace Uecode\Bundle\QPushBundle\Provider;c
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks wrong.

@k-k
Copy link
Contributor

k-k commented Mar 7, 2015

@pmartelletti - the SQS api for creating queues is idempotent, so it will never replace or destroy an existing queue. It will just return the queue url for the existing queue. As seen in their docs.

That being said, the createQueue() method is explicit and shouldn't need to worry about the existence of the queue because of the idempotent nature of the sqs create endpoint. This is not really any more expensive then the exists method if its a queue qpush isn't yet aware of (empty memory or file cache).

Has this actually caused issues for you?

@pmartelletti
Copy link
Author

@kmfk actually, yes! So, imagine that in the firts call of create the queue was made, the second call will throw an exception, saying that a queue with that name already exists. I was not trying to make it "less expensive", just make it work. 😄

But, to be honest, having a look at this PR, seems that's what needs to be fixed, and not the check of existing queues.

So, at the moment, yes, the code throws an exception if the queue exists and the topic does not, or if neither of them exists (so, actually, you won't be able to work with push notifications), at least you modify the code acorrding to my PR or this PR #63.

So I think that need to apply either of the PR's. What do you thing?

@k-k
Copy link
Contributor

k-k commented Dec 18, 2015

@pmartelletti - This is obviously very old, but I cleaned up #63 and merged that in, which should have resolved this issue. Thanks!

@k-k k-k closed this Dec 18, 2015
@pmartelletti
Copy link
Author

@kmfk indeed is old, but I rember that applying #63 the issue was fixed. 😄

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants