Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

MAINT: Refactor auditrecords.go to loop only once #1570 #1658

Merged
merged 15 commits into from
Aug 11, 2022

Conversation

tlimoncelli
Copy link
Contributor

@tlimoncelli tlimoncelli commented Aug 4, 2022

Fixes #1570

The old AuditRecord does a lot of unneeded work. It loops through every record once for each test. This change loops through all the records once, calling checking functions appropriate to that record type. It also makes the format of the initializers readable.

I timed it on a small dataset and got a 1-3% improvement in total run time, I suspect that if I timed the function itself it would be more for providers that have many tests.

@tlimoncelli tlimoncelli marked this pull request as draft August 4, 2022 21:05
@tlimoncelli tlimoncelli marked this pull request as ready for review August 9, 2022 01:38
@tlimoncelli
Copy link
Contributor Author

@tresni @pragmaton @mikenz @Deraen @onlyhavecans @vojtad @Papakai @svenpeter42 @tlimoncelli @kordianbruck @costasd @MisterErwin @pgaskin

Hey folks! I rewrote the record audit stuff to be significantly faster and less verbose. I believe I've converted the old code properly. Please take a look.

Copy link
Contributor

@costasd costasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for NS1

@onlyhavecans
Copy link
Contributor

Looks good for DNSimple!

@MisterErwin
Copy link
Contributor

LGTM for the rwth provider (although it is a blatant copy of the other providers)

@tlimoncelli tlimoncelli changed the title Improve AuditRecord speed MAINT: Refactor auditrecords.go to loop only once #1570 Aug 11, 2022
@tlimoncelli
Copy link
Contributor Author

Flakey tests. They passed other times.

@tlimoncelli tlimoncelli merged commit 31723ad into master Aug 11, 2022
@tlimoncelli tlimoncelli deleted the tlim_fastertxtaudit branch August 11, 2022 21:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAINT: Refactor auditrecords.go to do everything in one loop
4 participants