Switching to boto3 only #693

revmischa · 2017-02-15T09:14:53Z

I have removed all traces of boto2 that I could find. I got async message signing and parsing more or less working. Probably still needs some cleanup and testing.
Much farther along now on the journey to boto3.

… and signing working.

georgepsarakis · 2017-02-15T19:25:22Z

kombu/transport/SQS.py

@@ -160,15 +152,23 @@ def _reset_cycle(self):

    def entity_name(self, name, table=CHARS_REPLACE_TABLE):
        """Format AMQP queue name into a legal SQS queue name."""
-        return text_t(safe_str(name)).translate(table)


@revmischa why is there a differentiation for queues with suffix .fifo?

@jseutter did that not me.
.fifo suffix means it's a FIFO queue, i.e. retains ordering. this is a new feature in SQS. non-FIFO queues don't preserve ordering.

Is that a necessary change in the scope of removing boto2? I am pointing this out because the changes are already very extensive. I think it would be advisable to keep the diff to a minimum.

I think FIFO support is absolutely a requirement. Otherwise Kombu will likely break if someone tries to use a new-style queue. However I did not implement this, I'm including the changes from @jseutter in my PR because they're based off of his branch.

So, it is basically an enhancement, adding SQS FIFO support :) . I believe it belongs to a separate Pull Request, but I will not insist any further.

I don't disagree with that. It's not my commit though.

georgepsarakis · 2017-02-15T19:28:02Z

requirements/extras/sqs.txt

@@ -1,2 +1,3 @@
-boto>=2.8
+boto3
+botocore


I think boto3 already depends on botocore. Also, perhaps it would be a good idea to specify (at least) a minimum version?

Yes, good point
17f4ab0

georgepsarakis · 2017-02-15T19:28:58Z

t/unit/async/aws/case.py

@@ -8,6 +8,7 @@

 @skip.if_pypy()
 @skip.unless_module('boto')


Perhaps this line should be removed?

georgepsarakis · 2017-02-15T20:37:21Z

I found this project that seems quite active. Perhaps it could be used for integration tests? What do you think? Could be useful for Celery testing as well.

revmischa · 2017-02-15T23:41:05Z

Maybe usesful @georgepsarakis I'm just trying to get all the wacky original SQS tests passing again which is no small feat. They were extremely coupled to the boto2 behaviors.

revmischa · 2017-03-07T00:38:15Z

@jseutter check out the work I did and let me know if you have any questions. i'm not planning to spend any more time on this right this second. theres no boto2 left anywhere.

auvipy · 2017-03-07T04:34:41Z

@revmischa thanks for the work. what reamain in your thought?

@jseutter review and comment what need to be change

revmischa · 2017-03-07T04:41:23Z

I managed to get all the tests passing, but I wasn't sure if all the modifications I made were proper. Really the code needs review by someone who understands Kombu better than me. I was trying to perform some major surgery without having any idea of how anything was supposed to fit together. I changed everything I could find but I wasn't really sure how to test all the cases. Probably the channel creation (esp with FIFO support) needs proper testing. I also made big changes to the message serialization/deserialization stuff.
tl;dr I don't know what's left to do. I know zero about Kombu and probably broke some things.

georgepsarakis · 2017-04-13T18:33:12Z

@revmischa could you please rebase?

revmischa · 2017-04-13T18:43:44Z

You rebase it. I gave up on Celery and wrote something better using PostgreSQL's SELECT ... FOR UPDATE SKIP LOCKED feature.

alukach · 2017-04-14T03:24:09Z

@revmischa So are you (and the rest of the @jetbridge organization) formally abandoning this PR?

I'm interested in getting this merged. Getting away from boto2 will be a large win for Kombu moving forward as boto2:

doesn't support Python3.4 (Support Python 3.4 boto/boto#3713)
doesn't support FIFO queues (Switch Boto2 to Boto3 for SQS messaging #678, Does Celery Support SQS FIFO? celery#3690)
has a broken endpoints file (Celery 4.0.0 SQS -> "Credential should be scoped to a valid region, not 'queue'." celery#3672, Update SQS endpoints (Fix SignatureDoesNotMatch Error) boto/boto#3633, sqs region related signature fixed. boto/boto#3688)
appears to be dead/overwhelmed (at time of writing, there are 359 open PRs and no activity for the past month, Update SQS endpoints (Fix SignatureDoesNotMatch Error) boto/boto#3633 has been open since Oct 2016)

@georgepsarakis What are your feelings about this PR? How far is it from completion? What else needs to be done before you'd feel comfortable merging? I'm just stepping into this code right now but am willing to put in some hours if need be. I'm running off of the PR branch on my local machine and it seems to be close to working (my message bodies appear to be encoded in Base64, need to figure out why that is, quite possibly not related to this PR).

@jseutter Do you have any thoughts on what needs to be done for this to be called "complete"?

revmischa · 2017-04-14T03:27:27Z

I'll merge in changes if you want but I don't have anything further to contribute to this PR personally.

auvipy · 2017-04-14T05:16:48Z

a rebase would be fine

auvipy · 2017-04-14T05:22:57Z

i rebased and squashed. plz test and report bugs

georgepsarakis · 2017-04-14T05:39:57Z

@revmischa thanks for your efforts on this PR 😃 .

@alukach @auvipy I will try to add integration tests in Celery using Atlassian LocalStack, which I recently discovered, and has SQS support.

auvipy · 2017-04-14T06:12:02Z

that would be really awesome!

alukach

Trying this out on my machine, I think there are some small issues around encoding to/from base64 when using Python3.4

alukach · 2017-04-14T04:53:53Z

kombu/async/aws/connection.py

+
+        # print(prepared_request.url)
+        # print(prepared_request.headers)
+        # print(prepared_request.body)


A bit of cleanup needed here

alukach · 2017-04-14T04:54:56Z

t/integration/tests/test_SQS.py

@@ -8,6 +8,7 @@
 @skip.unless_environ('AWS_ACCESS_KEY_ID')
 @skip.unless_environ('AWS_SECRET_ACCESS_KEY')
 @skip.unless_module('boto')


No longer needed (I believe)

alukach · 2017-04-14T05:34:45Z

kombu/async/aws/sqs/message.py

-    """Json serialized message."""
+    def encode(self, value):
+        """Encode/decode the value using Base64 encoding."""
+        return base64.b64encode(value).decode('utf-8')


This appears to break with Python3.4. base64.b64encode() expects a bytes-like object.

I think this line should be return base64.b64encode(value.encode()).decode()

alukach · 2017-04-14T06:15:30Z

kombu/transport/SQS.py


    def _message_to_python(self, message, queue_name, queue):
-        payload = loads(bytes_to_str(message.get_body()))
+        payload = loads(bytes_to_str(message['Body']))


At this moment, won't the message be in in base64?

Recommend changing this to:

body = base64.b64decode(message['Body'].encode()) payload = loads(bytes_to_str(body))

@revmischa I know you're signing off on this PR, however would you be able to comment on this? I was unable to get the code running with modifying the code as mentioned above.

I am not sure why base64 was used, if I recall correctly it was done in the boto2 version and I didn't change it. I was developing on python3.5 but I definitely had some issues with the encoding/decoding differences between py2 and p3 and I suggest anything you can do to simplify all of that is a good idea.

@alukach you can see that the base64 encoding is related to the Message implementation of boto here.

Yup, I now understand that the boto2 did the base64 encoding automatically. Will make a PR in a few...

alukach · 2017-04-14T06:15:50Z

kombu/transport/SQS.py

@@ -209,14 +224,14 @@ def _message_to_python(self, message, queue_name, queue):
                delivery_info = {}
                properties = {'delivery_info': delivery_info}
                payload.update({
-                    'body': bytes_to_str(message.get_body()),
+                    'body': bytes_to_str(message['Body']),


... and then use the body variable here.

alukach · 2017-04-14T06:38:03Z

@auvipy @georgepsarakis Thanks for rebasing, however I think the merge may have been a bit pre-emptive. Did anyone try to test this on their machines? I was unable to get workers to consume tasks without modifying the code.

georgepsarakis · 2017-04-14T06:51:53Z

@alukach would you mind opening a separate Pull Request with your changes? It would be valuable to merge fixes since you seem to be testing with SQS directly. Thanks!

alukach · 2017-04-14T06:56:34Z

@georgepsarakis PR made. Take a look.

audiolion · 2017-04-14T13:15:32Z

Yay! 🎉

@revmischa Thank you thank you for all the initial work done to support SQS!! it is much appreciated

Thank you @alukach and kombu team for iterating on this patch, this was holding me back from using celery + sqs!

krzysztofwolski · 2017-04-18T11:37:41Z

Thanks! I was waiting for this merge :). When can I expect release with those changes?

This change pins Kombu to an unreleased commit that fixes Celery 4 to work properly with SQS. While testing on the dev environment it became clear that while the celery worker was able to connect to the SQS queue it wasn't actually receiving any messages off it. The issue linked below [1] shows that Celery 4.0.2 does not work correctly with SQS. While there is a fix that that has been merged on Kombu [2] this has not made it into a release yet which is why this change pins Kombu to a commit on master. [1] celery/celery#3672 [2] celery/kombu#693

This change pins Kombu to an unreleased commit that fixes Celery 4 to work properly with SQS. While testing on the dev environment it became clear that while the celery worker was able to connect to the SQS queue it wasn't actually receiving any messages off it. The issue linked below [1] shows that Celery 4.0.2 does not work correctly with SQS. While there is a fix that that has been merged on Kombu [2] this has not made it into a release yet which is why this change pins Kombu to a commit on master. I have tested the Kombu commit with a minimal Django 1.11 app using SQS as the broker. [1] celery/celery#3672 [2] celery/kombu#693

Kombu is a library that celery uses under the hood. Kombu is pinned to a specific commit that brings in fixes for SQS - see celery/kombu#693. Kombu v4.0.2 (which ships with celery v4.0.2) doesn't work with SQS due to problems with their use of boto2, so Kombu migrated to boto3 - We're waiting for that to get a release version, and then to get a new version of celery that pulls that in. Until that point, we should override the kombu version to get these SQS fixes. Additionally, we don't need boto2 any more :)

Jerry Seutter and others added 19 commits December 29, 2016 22:12

Switch Boto2 to Boto3 for SQS messaging

b1169d7

Fixed region support

fad9250

Add SQS FIFO queue support

39b7bd3

Add sensible defaults for message attributes

04ee160

Asynchronous support, plus boto3 for region endpoint lookups

c270e92

Clean up imports

1e4b1e3

Fix Python 2 support

66870de

Fix receive_message tests

5f20c80

Reformat docstring

8a6fb0e

boto3 import changes for CI

bc8e108

skip tests if boto3 not installed

759e09e

skip tests if boto3 not installed

c4b5564

flake8

90c23b0

Merge commit 'b1169d7f5e564bae85f0026af0e58d2457d56ae2'

1389269

Merge remote-tracking branch 'pr/boto3-service-resource'

cd29d56

noboto

2d492a4

ditching boto2. got queue URL fetching, async HTTP request generation…

57ea7f1

… and signing working.

request signing working kinda

fbab896

async parsing of SQS message response more or less working

f8f568a

revmischa mentioned this pull request Feb 15, 2017

Switch Boto2 to Boto3 for SQS messaging #678

Closed

revmischa added 3 commits February 15, 2017 01:20

botocore sqs dep

053db8c

ripping out more old boto2 stuff

bf8148e

removing tests that are no longer valid with boto3/SQS

aebabd8

georgepsarakis reviewed Feb 15, 2017

View reviewed changes

revmischa added 2 commits February 15, 2017 12:26

fix boto3 dep, min version and no botocore

17f4ab0

no boto2 for test

a446e9d

cleaning up some SQS tests. fixing header parsing of response to msg

b4911a4

alukach mentioned this pull request Apr 14, 2017

Celery 4.0.0 SQS -> "Credential should be scoped to a valid region, not 'queue'." celery/celery#3672

Closed

georgepsarakis added the Status: Needs Rebase label Apr 14, 2017

Merge branch 'master' into noboto2

5f66684

auvipy merged commit 129a9e4 into celery:master Apr 14, 2017

alukach reviewed Apr 14, 2017

View reviewed changes

alukach mentioned this pull request Apr 14, 2017

Bugfix: Support Python 3.4, decode from base64 when converting messag… #714

Merged

georgepsarakis mentioned this pull request Apr 16, 2017

Allow endpoint URL to be specified for SQS transport #715

Merged

3 tasks

This was referenced May 5, 2017

Prevent SQS from defaulting to localhost hostname #733

Merged

[4.0.0.rc2] Connection error to AWS SQS celery/celery#3221

Closed

robyoung mentioned this pull request Jun 28, 2017

Fix Celery 4 upgrade with SQS ministryofjustice/manchester_traffic_offences_pleas#354

Merged

dws-sarthak mentioned this pull request Oct 8, 2017

Does Celery Support SQS FIFO? celery/celery#3690

Closed

Switching to boto3 only #693

Switching to boto3 only #693

Conversation

revmischa commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgepsarakis commented Feb 15, 2017

revmischa commented Feb 15, 2017

revmischa commented Mar 7, 2017

auvipy commented Mar 7, 2017

revmischa commented Mar 7, 2017

georgepsarakis commented Apr 13, 2017

revmischa commented Apr 13, 2017

alukach commented Apr 14, 2017 • edited Loading

revmischa commented Apr 14, 2017

auvipy commented Apr 14, 2017

auvipy commented Apr 14, 2017

georgepsarakis commented Apr 14, 2017

auvipy commented Apr 14, 2017

alukach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alukach commented Apr 14, 2017

georgepsarakis commented Apr 14, 2017

alukach commented Apr 14, 2017 • edited Loading

audiolion commented Apr 14, 2017

krzysztofwolski commented Apr 18, 2017

alukach commented Apr 14, 2017 •

edited

Loading

alukach commented Apr 14, 2017 •

edited

Loading