Batch DNS refresh requests on host renames - #3181
Conversation
f5944e0 to
2696fc6
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on gbrodman).
core/src/main/java/google/registry/dns/RefreshDnsOnHostRenameAction.java line 77 at r1 (raw file):
private void runDnsRefresh() { ImmutableSet<VKey<Domain>> linkedDomainKeys = tm().transact(
The code gets a decent bit simpler if you don't combine basic error checking in the same transaction as loading all the linked domain keys -- thoughts on doing it that way? Then you shouldn't need this new exception class or special exception handling. And considering how infrequently this is called, having a single additional transaction is a fair trade-off for simpler code.
Some hosts can have more than 100k domains linked to them so we probably don't want to insert all those entries at once.
2696fc6 to
0f69b77
Compare
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on CydeWeys).
core/src/main/java/google/registry/dns/RefreshDnsOnHostRenameAction.java line 77 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
The code gets a decent bit simpler if you don't combine basic error checking in the same transaction as loading all the linked domain keys -- thoughts on doing it that way? Then you shouldn't need this new exception class or special exception handling. And considering how infrequently this is called, having a single additional transaction is a fair trade-off for simpler code.
yeah that's fair, one extra transaction isn't that big of a deal especially since we're batching to create more transactions anyway. Done.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.
Some hosts can have more than 100k domains linked to them so we probably don't want to insert all those entries at once.
As far as I'm aware this hasn't been an issue in practice, but we should still do it just in case.
Self assessment D.5 number 16
This change is