This repository has been archived by the owner on Feb 16, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 10
Improve performance of creating access rules #236
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SimonBarendse
changed the title
Refactor, test and implement multithreadding for the reencryption functionality from 'pkg/secrethub/acl.go'
Improve performance of creating access rules
Apr 1, 2021
Changing naming and test struct format to be consistent throughout the repository.
Just printing the error isn't sufficient. In case of an error in these situations, we shouldn't continue, as it results in a nil-pointer derference.
Separating testcases like this gives lots of benefits: - When a test fails, the name of the case that fails is given. - Test reports show all cases ran - When one case fails, the others are still also ran, to see if they succeed.
SimonBarendse
approved these changes
Apr 1, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet stuff! 👌
I've tested this on a huge repo (2000 secrets). On develop
it takes a whopping 231s to create an access rule for the root, with these changes applied it takes 28s. Although that's still a long while (the encryption itself also takes long), it's a huge improvement! Thanks for adding this! 💙
I've pushed a couple of changes to the test suite. Could you have a look at them? I've added some explanation in the commit messages. If you agree with what I've done, this is ready to go 🚀
Thanks for the touch ups! Everything looks good to me too! I'll merge now. |
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to check if the optimisation is effectual, I have run the following script, which populated the priorly created
optimization-test
directory with 1000 secrets:I have then replaced the CLI's SDK dependency by pasting the following in CLI project's
go.mod
:and then I rebuilt the CLI.
NB, I was on the
feature/refactor-and-test-acl
branch on the SDK.The effect of the pull request can be observed by comparing the running times of an
acl set
command run on a directory with a large number of secrets (such as the one created by the script above), before and after the change, which, in my experience, differed by quite a large factor.If anyone considers this necessary, I will measure the actual times for both the before and after cases, making use of the
duration
library.