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

Switch Boto2 to Boto3 for SQS messaging #678

Closed
wants to merge 13 commits into from
Closed

Switch Boto2 to Boto3 for SQS messaging #678

wants to merge 13 commits into from

Conversation

jseutter
Copy link
Contributor

@jseutter jseutter commented Dec 30, 2016

This PR replaces Boto 2 with Boto3 for SQS messaging. I did it for a couple of reasons. First, my application needs FIFO queues which are unsupported in Boto2, and second, the AWS regions dict in Boto 2 is out of date and can no longer connect to the us-east-2 region.

The code is a work in progress and not yet ready to be merged. I'm posting it here to ask you to take a look at it and give feedback. Unresolved issues:

1. AFAIK, I only tested synchronous functionality. How do I test the async stuff? FIXED
2. I need to fix region support. This will be relatively easy, just a matter of doing it. FIXED
3. I dropped some behavior that seemingly tried to make authentication errors more informative. At least, that's what I think it did. Do you want this behavior preserved? If so, I'll add it back in. I'm not sure how hard or easy it will be. REMOVED
4. Add FIFO queue support. ADDED

I started fixing up the diff posted on issue #592, but switched directions when I realized that diff was using Boto3's low level SQS.Client interface. The existing code is a much better fit with the higher level SQS.Resource and SQS.Queue interfaces provided by Boto3.

If not in the next couple of days, could someone respond with an estimate of when you might look at this PR? I'm not intending to be pushy, I just want to set my own expectations. :)

Thanks for all your awesome work!

Edit: see comment below

@auvipy
Copy link
Member

auvipy commented Dec 30, 2016

cool

@jseutter
Copy link
Contributor Author

This PR is ready for testing. The unit tests pass, and Celery works in synchronous and asynchronous modes. I ended up keeping the dependency on boto2 for the async stuff because switching that over to boto3 (botocore) is going to be difficult. There are some libraries that reimplement botocore as async and it might be useful to use one of them in the future.

The boto3 code is being used for region lookups, bypassing the boto2 out-of-date region information.

Can you review?

@auvipy
Copy link
Member

auvipy commented Jan 12, 2017

travis is failing

@jseutter
Copy link
Contributor Author

jseutter commented Jan 13, 2017

I'll take a look. Also found that messages will stop processing after a period of time in async mode. I must have an issue with event loop stuff.

@jseutter
Copy link
Contributor Author

jseutter commented Jan 23, 2017

Tests fixed. The problem where the workers stop happens with master as well. It seems to be related to my setup that connects to multiple queues and routes messages between them. I'll keep debugging, but I'd like to create a separate branch for the issue.

@jseutter
Copy link
Contributor Author

jseutter commented Feb 6, 2017

Are my changes suitable to be merged into master, or are there things you would like to see changed?

@georgepsarakis
Copy link
Contributor

@jseutter can you please check if this PR also fixes this issue reported in Celery? Thanks!

Copy link
Contributor

@revmischa revmischa left a comment

Choose a reason for hiding this comment

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

Please merge this and release!

@revmischa
Copy link
Contributor

boto2 SQS straight up does not work. I get this error when trying to use it:
Credential should be scoped to a valid region

@adamszeptycki
Copy link

Please merge this and release if everything is ok with that. I have the same problem in my project.

@auvipy
Copy link
Member

auvipy commented Feb 8, 2017

@ask need your attention

@jseutter
Copy link
Contributor Author

@georgepsarakis - Yes, I was seeing the same error as that celery issue, so unless there is another reason for the same error, this PR fixes it. I'll comment there as well.

@revmischa
Copy link
Contributor

I am attempting to continue work on @jseutter 's boto3 migration. I'm trying to remove all traces of boto2 and a lot of this old cruft. It is slow going. If anyone wants to help out please let me know. I'll push what I have if I get anywhere. The async connection stuff is rather difficult to shoehorn boto3 into.

@revmischa
Copy link
Contributor

More work on migrating to boto3 SQS based off of @jseutter 's changes:
#693

@audiolion
Copy link

I would love to see this merged in!

@alukach alukach mentioned this pull request Apr 14, 2017
@auvipy
Copy link
Member

auvipy commented Apr 14, 2017

merged in 129a9e4

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.

6 participants