feat: convert to use AWS boto3 #1061

jrconlin · 2017-10-25T19:28:38Z

codecov-io · 2017-10-27T00:55:17Z

Codecov Report

Merging #1061 into master will decrease coverage by 0.05%.
The diff coverage is 98.19%.

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
- Coverage   99.73%   99.67%   -0.06%     
==========================================
  Files          56       56              
  Lines        8904     8999      +95     
==========================================
+ Hits         8880     8970      +90     
- Misses         24       29       +5

Impacted Files	Coverage Δ
autopush/tests/test_websocket.py	`100% <100%> (ø)`	⬆️
autopush/web/webpush.py	`100% <100%> (ø)`	⬆️
autopush/tests/test_db.py	`100% <100%> (ø)`	⬆️
autopush/websocket.py	`100% <100%> (ø)`	⬆️
autopush/tests/__init__.py	`100% <100%> (ø)`	⬆️
autopush/web/health.py	`100% <100%> (ø)`	⬆️
autopush/tests/test_web_base.py	`100% <100%> (ø)`	⬆️
autopush/web/base.py	`100% <100%> (ø)`	⬆️
autopush/db.py	`100% <100%> (ø)`	⬆️
autopush/tests/test_integration.py	`100% <100%> (ø)`	⬆️
... and 3 more

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 fa47b5c...0278472. Read the comment docs.

pjenvey · 2017-10-30T17:58:26Z

autopush/db.py

@@ -92,6 +95,16 @@
 TRACK_DB_CALLS = False
 DB_CALLS = []

+# See https://botocore.readthedocs.io/en/stable/reference/config.html for
+# additional config options
+DYNAMODB = boto3.resource(


this and client are ultimately stateful (mutable) objects underneath: so I'd say they should not have constant-like upper case names

pjenvey · 2017-10-30T18:04:01Z

autopush/db.py

@@ -43,16 +44,17 @@
    attrib,
    Factory
 )
-from boto.exception import JSONResponseError
+
 from boto.dynamodb2.exceptions import (


We still raise ItemNotFound but AFAICT the other 2 boto2 exceptions aren't in use any longer

IIUC, ProvisionedThroughputExceededException is in botocore and still could be returned. ConditionalCheckFailedException appears to be wrapped into ClientError,so we can probably remove it.

Nope, it looks like even ProvisionedThroughputExceededException becomes a ClientError (or one of those dynamic subclass of it) now. This boto2 one is defined in that boto.dynamodb2.exceptions module: boto3/botocore don't use boto2 at all AFAICT

e.g. boto/boto3#597 (comment)

Revised the errors to be ClientErrors and added handling for the special cases.

pjenvey · 2017-10-30T18:44:40Z

autopush/db.py

+            }
+        )
+    except ClientError as ex:  # pragma nocover
+        if "UnknownOperationException" in ex.message:


Tempted to restrict this case to only during tests

I dunno. We're eating an error. I'm not a huge fan of this either, but the other option is that we introduce a potentially complicated test environment check system (mock, env check, passed flag even for integration, etc.). The try block is limited to the one function and the significant return, so I think it may be OK to do this.

Yea, probably not totally necessary. I think UnknownOperationException is a pretty clear case that we should not ever see in prod. It does show up as the exception "type" (response['Error']['Code'] == 'UnknownOperationException') so we can check that a little more explicitly here at least

pjenvey · 2017-10-30T21:12:24Z

autopush/db.py

@@ -238,8 +345,8 @@ def preflight_check(message, router, uaid="deadbeef00000000deadbeef00000000"):
    # Verify tables are ready for use if they just got created
    ready = False
    while not ready:
-        tbl_status = [x.describe()["Table"]["TableStatus"]
-                      for x in [message.table, router.table]]
+        tbl_status = [x.table_status()


can collapse this newline now

pjenvey · 2017-10-30T21:45:27Z

autopush/db.py

-                                          consistent=True, limit=limit))
-
+        response = self.table.query(
+            KeyConditions={


KeyConditions is legacy now, they're recommending KeyConditionExpressions:

http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/LegacyConditionalParameters.html

I guess this becomes KeyConditionExpression=Key('uaid').eq(uaid.hex) & Key('chidmessageid').lt("02")?. don't quote me on that

pjenvey · 2017-10-30T22:00:18Z

autopush/db.py

+        except ClientError as ex:
+            # ClientErrors are generated by a factory, and while they have a
+            # class, it's dynamically generated.
+            if 'ConditionalCheckFailedException' == ex.__class__.__name__:


Since they recommend checking against [Error']['Code'] like you did above for ResourceNotFoundException, let's just standardize on that. The test can raise ClientError({'Error': {'Code': 'ConditionalCheckFailedException'}})

pjenvey · 2017-10-30T22:05:10Z

autopush/db.py

-        return self.table.delete_item(uaid=huaid,
-                                      expected={"uaid__eq": huaid})
+        try:
+            self.get_uaid(uaid)


Why add the get_uaid call here? It could be dangerous as get_uaid potentially calls drop_user

Pretty much for the reason noted. We only delete UAIDs that already exist. If get_uaid deletes the uaid, then it was already scheduled for deletion and returning False is acceptable. Either way, it's deleted.

This can turn into infinite recursion w/ an invalid record, though. See the case in test_db.test_incomplete_uaid -- if you disable the mock of drop_user in there it'll blow up the stack

pjenvey · 2017-10-30T22:10:05Z

autopush/tests/__init__.py

+    autopush.db.DYNAMODB = boto3.resource(
+        'dynamodb',
+        config=conf,
+        endpoint_url=os.getenv("AWS_LOCAL_DYNAMODB", "http://127.0.0.1:8000")


I think we'll have to do something about credentials here (hardcode?) so tests run locally without needing to set them

pjenvey · 2017-10-30T22:12:10Z

automock/boto.cfg

@@ -1,9 +1,13 @@
-[Credentials]


Seems like we should kill this entire file now, boto3 only reads "Credentials" from a BOTO_CONFIG (which it considers an old boto2 config file).

Also if we're no longer able to configure our local dynamodb through this file our docker-compose setup will no longer work. I think that's a whole separate issue entirely from this PR, though

Yeah, going to hardcode the credentials, since they're bogus anyway and we only need them for the test.

pjenvey · 2017-10-31T21:50:59Z

autopush/utils.py

@@ -301,6 +302,8 @@ class WebPushNotification(object):
    # Whether this notification should follow legacy non-topic rules
    legacy = attrib(default=False)  # type: bool

+    expry = attrib(default=MAX_EXPRY)  # type: int


Is this expry attribute here even necessary? We seem to always fill the expry headed towards the database from the ttl attribute. Only thing I see this doing is propagating it over the connection node via JSON, but it's not even used there

was being a bit pre-emptive. will remove for now.

Actually, going to leave it. We use it for several tests and it identifies as a keyword parameter.

bbangert · 2017-10-31T21:30:12Z

autopush/__init__.py

@@ -1 +1,4 @@
 __version__ = '1.39.0'  # pragma: nocover
+
+# Max DynamoDB record lifespan (~ 30 days)
+MAX_EXPRY = 2592000  # pragma: nocover


bbangert · 2017-10-31T21:34:31Z

autopush/db.py

-        return repr(uaid_data.items())
-    else:
-        return repr(uaid_data)
+    return repr(uaid_data)


Maybe we just drop this function then and use repr? This only existed to unify the if/else statement.

bbangert · 2017-10-31T21:54:16Z

autopush/db.py

+                'KeyType': 'RANGE'
+            }],
+        AttributeDefinitions=[{
+                'AttributeName': 'uaid',


Why the different style of [{ here than above? Should be nicer when we use an auto-formatter I guess.

bbangert · 2017-10-31T21:57:36Z

autopush/db.py

+            'ReadCapacityUnits': read_throughput,
+            'WriteCapacityUnits': write_throughput,
+        },
+        GlobalSecondaryIndexes=[{


To [{ or [\n\t{

bbangert · 2017-11-01T01:41:03Z

autopush/db.py

@@ -333,26 +441,31 @@ def generate_last_connect_values(date):
            yield int(val)


-def list_tables(conn):
+def list_tables(client=CLIENT):


Mutable defaults generally aren't great, it looks a little odd.

bbangert · 2017-11-01T18:29:35Z

autopush/db.py

@@ -256,6 +363,7 @@ def preflight_check(message, router, uaid="deadbeef00000000deadbeef00000000"):
        update_id=message_id,
        message_id=message_id,
        ttl=60,
+        expry=_expry(60),


Is there some reason we can't include the i to make it a real word? It is supposed to be expiry, right?

pjenvey · 2017-11-02T23:15:09Z

autopush/main_argparse.py

@@ -115,7 +115,7 @@ def _obsolete_args(parser):
    parser.add_argument('--external_router', help='OBSOLETE')
    parser.add_argument('--max_message_size', type=int, help="OBSOLETE")
    parser.add_argument('--s3_bucket', help='OBSOLETE')
-    parser.add_argument('--senderid_expry', help='OBSOLETE')
+    parser.add_argument('--senderid_expiry', help='OBSOLETE')


pjenvey · 2017-11-03T17:41:18Z

autopush/utils.py

@@ -335,6 +335,12 @@ def generate_message_id(self, fernet):
        self.update_id = self.message_id
        return self.message_id

+    @classmethod
+    def calc_expiry(cls, ttl=0):


this is now dead code: kill

pjenvey · 2017-11-03T17:46:24Z

autopush/db.py

-        return self.table.delete_item(uaid=huaid,
-                                      expected={"uaid__eq": huaid})
+        try:
+            self.get_uaid(uaid)


This can turn into infinite recursion w/ an invalid record, though. See the case in test_db.test_incomplete_uaid -- if you disable the mock of drop_user in there it'll blow up the stack

pjenvey

just a couple more things, most importantly the drop_user/get_uaid issue

bbangert · 2017-11-03T19:38:30Z

autopush/db.py

@@ -92,6 +94,16 @@
 TRACK_DB_CALLS = False
 DB_CALLS = []

+# See https://botocore.readthedocs.io/en/stable/reference/config.html for
+# additional config options
+g_dynamodb = boto3.resource(


What does the g_ mean?

Well, i wanted to make sure that there was no potential for symbol conflict, and I picked "g_" because it's effectively a global.

bbangert · 2017-11-03T19:46:30Z

autopush/db.py

-                                          consistent=True, limit=limit))
+        response = self.table.query(
+            KeyConditionExpression=Key('uaid').eq(hasher(uaid.hex))
+            & Key('chidmessageid').gt(sortkey),


nit: maybe indent so only the keys line up.

flake8 complains about that, but I can move the & to the previous line.

can also wrap the whole statement in parens if that helps

Issue #1050 Closes #1049

jrconlin requested review from pjenvey and bbangert October 25, 2017 19:28

jrconlin added the in progress label Oct 25, 2017

jrconlin force-pushed the feat/1049 branch 6 times, most recently from 489ef4a to e96303d Compare October 25, 2017 23:27

jrconlin removed request for pjenvey and bbangert October 26, 2017 15:41

jrconlin force-pushed the feat/1049 branch 9 times, most recently from fd99c2b to 5566b79 Compare October 27, 2017 00:39

jrconlin requested a review from bbangert October 27, 2017 01:24

jrconlin force-pushed the feat/1049 branch from 8c228c9 to 354530c Compare October 27, 2017 01:33

jrconlin requested a review from pjenvey October 27, 2017 01:41

pjenvey suggested changes Oct 30, 2017

View reviewed changes

pjenvey reviewed Oct 30, 2017

View reviewed changes

pjenvey reviewed Oct 31, 2017

View reviewed changes

jrconlin force-pushed the feat/1049 branch 2 times, most recently from f08e679 to 5e239e4 Compare November 1, 2017 00:17

bbangert reviewed Nov 1, 2017

View reviewed changes

jrconlin force-pushed the feat/1049 branch 4 times, most recently from 34cf8f5 to e0a9a85 Compare November 2, 2017 17:49

jrconlin requested a review from pjenvey November 2, 2017 18:36

jrconlin assigned bbangert Nov 2, 2017

jrconlin requested a review from bbangert November 2, 2017 18:37

jrconlin unassigned bbangert Nov 2, 2017

pjenvey reviewed Nov 2, 2017

View reviewed changes

jrconlin force-pushed the feat/1049 branch 3 times, most recently from 555d8a1 to 17f28ee Compare November 2, 2017 23:54

pjenvey reviewed Nov 3, 2017

View reviewed changes

pjenvey suggested changes Nov 3, 2017

View reviewed changes

jrconlin force-pushed the feat/1049 branch from 17f28ee to d9912d3 Compare November 3, 2017 18:42

bbangert reviewed Nov 3, 2017

View reviewed changes

jrconlin force-pushed the feat/1049 branch 2 times, most recently from 782cb5f to 2a5d4ea Compare November 3, 2017 20:47

feat: convert to use AWS boto3

0278472

Issue #1050 Closes #1049

jrconlin force-pushed the feat/1049 branch from 2a5d4ea to 0278472 Compare November 3, 2017 20:49

pjenvey approved these changes Nov 3, 2017

View reviewed changes

bbangert approved these changes Nov 3, 2017

View reviewed changes

bbangert merged commit bc32908 into master Nov 3, 2017

bbangert deleted the feat/1049 branch November 3, 2017 22:00

bbangert removed the in progress label Nov 3, 2017

jrconlin mentioned this pull request Mar 6, 2018

Convert Message / Router tables to use TTL #1050

Closed

feat: convert to use AWS boto3 #1061

feat: convert to use AWS boto3 #1061

Conversation

jrconlin commented Oct 25, 2017

codecov-io commented Oct 27, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey Oct 31, 2017 • edited Loading

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

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

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

pjenvey 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

codecov-io commented Oct 27, 2017 •

edited

Loading

pjenvey Oct 31, 2017 •

edited

Loading