cmd/devp2p: use AWS-SDK v2 #22360
cmd/devp2p: use AWS-SDK v2 #22360
Conversation
| existing := make(map[string]recordSet) | ||
| err := c.api.ListResourceRecordSetsPages(&req, func(resp *route53.ListResourceRecordSetsOutput, last bool) bool { | ||
| for { |
There was a problem hiding this comment.
callback paginators are no longer implemented in v2, utilizing cursors instead. This refactor actually follows the same logical flow as an already existing pagination implementation
| if err := c.api.WaitUntilResourceRecordSetsChanged(wreq); err != nil { | ||
| return err | ||
| var count int | ||
| for { |
There was a problem hiding this comment.
waiters have been refactored in v2. They are no longer local extensions of the request.Waiter type. V2 does have support for waiters but limited to certain packages and functions.
Please see how the previous waiter was implemented
Like my implementation here, you can see that the waiter checks the status 60 times in 30 second increments
v2 waiter implementations are handled in a similar fashion
| @@ -109,21 +113,35 @@ func (c *route53Client) deploy(name string, t *dnsdisc.Tree) error { | |||
| batches := splitChanges(changes, route53ChangeSizeLimit, route53ChangeCountLimit) | |||
| for i, changes := range batches { | |||
There was a problem hiding this comment.
this loop can be optimized, mostly because the waiter is blocking (this is the case in the current version as well as in this PR).
Currently for every batch, it makes the change and then waits up to 30 minutes for the change to propagate before making the next change for the next batch. In a worse case scenario, this loop could block a command for hours.
My proposal is that either
-
we loop through the batch and make all of the changes, then block the thread until all changes are made. This way the bottleneck only exists on the AWS side of how fast changes are propagated
-
create a go-routine for each batch. This way each routine is responsible for making the change and then waiting for the propagation. The thread would lock until all of the routines have broken out of the wait loop. Like option 1, the bottleneck would only exist on the AWS side.
option 2 would be more elegant, both options would be a dramatic optimization. However I am not clear if this is necessary or desired. If it is I can include those changes in this PR or a different one (I can also create the optimization with the v1 sdk)
There was a problem hiding this comment.
If you want to make this change, I prefer option 1. There can be a lot of changes, and starting a goroutine for each has its own problems.
There was a problem hiding this comment.
@fjl I have implemented option 1. Propagation takes an average of 60 seconds, according to AWS, so now instead of waiting ~ 60-90 seconds per change, ideally it would take 60-90 seconds for the first change and then the rest of the waiters should be successful on the first request
qhenkart
force-pushed
the
aws-sdk-update
branch
from
b230bb3
to
eace772
Compare
|
@fjl probably we have to wait on this PR as a result of #22359 . The AWS-SDK-V2 requires Go 1.15 and if builds at 1.14 are still supported, then this PR should probably wait until 1.15 is the minimum build supported. Should I close and reopen when 1.14 is no longer supported? I have reopened another PR that recreates the same optimization that we discussed and applied here but for the v1 implementation |
|
I'll try this with Go 1.14, maybe it works anyway |
|
Actually, it looks like it does build with Go 1.14 (we have a 1.14 builder on Travis) |
This updates the DNS deployer to use AWS SDK v2. Migration is relatively seamless, although there were two locations that required a slightly different approach to achieve the same results. In particular, waiting for DNS change propagation is very different with SDK v2. This change also optimizes DNS updates by publishing all changes before waiting for propagation.
…for route53 (ethereum#22538) This PR fixes a regression introduced in ethereum#22360, when we updated to the v2 of the AWS sdk, which causes current crawler to just get the same first 100 results over and over, and get stuck in a loop.
The AWS SDK released the v2 with a number of performance and usability improvements.
I recommend perusing the announcement details
Migration is relatively seamless, although there were two locations that required a slightly different approach to achieve the same results. In both cases, the behavior mimics the v1 implementation verbatim. However these locations require a closer review and I think they've also exposed an opportunity for further optimization. I will leave comments in those locations to make the review easier