Update AGW to support the flat digest pattern, including storing and communicating digests#7340
Conversation
pull-request-size
bot
added
the
size/XL
|
Thanks for opening a PR! 💯 Please note that all commits must be signed off. This is enforced by the Howto
More infoPlease take a moment to read through the Magma project's
If this is your first Magma PR, also consider reading
|
wangyyt1013
force-pushed
the
agw-digest
branch
2 times, most recently
from
553901b to
1d7c7f2
Compare
pull-request-size
bot
added
size/L
wangyyt1013
force-pushed
the
agw-digest
branch
2 times, most recently
from
57630ca to
e72ade3
Compare
hcgatewood
left a comment
hcgatewood
left a comment
There was a problem hiding this comment.
Looks good, couple fixes and couple questions
hcgatewood
left a comment
hcgatewood
left a comment
There was a problem hiding this comment.
Looking good, couple cleanup items
| self._store.update_digest(cur_digest.md5_base64_digest) | ||
| return subscribers, no_updates |
There was a problem hiding this comment.
Make sure we test what happens when cloud sends down empty digest
There was a problem hiding this comment.
Currently the logic works as such: when cloud sends down an empty digest, the gateway will store the empty gateway and send it in its next request; however, if ListSubscribersRequest.Digest = "", then cloud will not enable the no_updates logic, but will send down the full subscriber list (as well as a new digest if it has one) as usual. Wondering if you think this is a reasonable logic? @hcgatewood
P.S. the "cloud not enabling no_updates logic when digest = """ is included in #7472
There was a problem hiding this comment.
Yes this is how it should work, makes sense. My note was just to verify that this is being tested
wangyyt1013
force-pushed
the
agw-digest
branch
2 times, most recently
from
fd69f22 to
77db6a3
Compare
hcgatewood
left a comment
hcgatewood
left a comment
There was a problem hiding this comment.
Lgtm
- Fix one small nit
- Wait to merge until we get both PRs ready and manually verified
wangyyt1013
force-pushed
the
agw-digest
branch
3 times, most recently
from
a2a3755 to
2b08f77
Compare
hcgatewood
left a comment
hcgatewood
left a comment
There was a problem hiding this comment.
Couple small changes then let's merge. Also need an agw review, msg @themarwhal
Signed-off-by: Yuanyuting Wang <[email protected]>
Signed-off-by: Yuanyuting Wang <[email protected]>
ardzoht
left a comment
ardzoht
left a comment
There was a problem hiding this comment.
Lgtm, left one comment but it can be added in a separate PR
| SUBSCRIBER_SYNC_FAILURE_TOTAL.inc() | ||
| return None | ||
| return None, None | ||
| logging.info( |
There was a problem hiding this comment.
For tuple structs like this I would recommend using a NamedTuple and explicitly name the parameters instead of returning a tuple here, it doesn't have to be in this PR you can add it as a TODO and add this later.
There was a problem hiding this comment.
sounds good, will do!
…communicating digests (#7340)
…communicating digests (magma#7340) Signed-off-by: Ramon Melero <[email protected]>
4 participants
Signed-off-by: Yuanyuting Wang [email protected]
Summary
subscriberdb/store/sqlite.pyto store and retrieve the current digest for the gatewaysubscriberdb/client.pyto fetch, receive, and update digests, as well as skip subscriber data resyncing whenno_updates=trueTest Plan
subscriberdb/tests/store/store_tests.pyandsubscriberdb/tests/client_tests.pysubscriberdbcloud in dockerAdditional Information