Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

Improve performance of creating access rules #236

Merged
merged 10 commits into from
Apr 1, 2021

Conversation

hculea
Copy link
Member

@hculea hculea commented Mar 31, 2021

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:

u, err := secrethub.NewClient()
if err != nil {
	fmt.Print(err.Error())
}

iterations := 1000
for i := 0; i < iterations; i++ {
	_, err := u.Secrets().Write(fmt.Sprintf("<SECRETHUB NAMESPACE>/optimization/optimization-test/secret%d", i), []byte(fmt.Sprintf("value%d", i)))
	if err != nil {
		fmt.Print(err.Error())
	}
}

I have then replaced the CLI's SDK dependency by pasting the following in CLI project's go.mod:

replace (
    github.com/secrethub/secrethub-go => <RELATIVE PATH TO secrethub-go FOLDER>  // in my case, ../secrethub-go
)

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.

@hculea hculea marked this pull request as ready for review March 31, 2021 11:40
@SimonBarendse 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.
Copy link
Member

@SimonBarendse SimonBarendse left a 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 🚀

@hculea
Copy link
Member Author

hculea commented Apr 1, 2021

Thanks for the touch ups! Everything looks good to me too! I'll merge now.

@hculea hculea merged commit 4e0215d into develop Apr 1, 2021
@SimonBarendse SimonBarendse deleted the feature/refactor-and-test-acl branch April 1, 2021 17:48
@SimonBarendse SimonBarendse mentioned this pull request Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants