Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Layout template #476

Merged
merged 11 commits into from
Feb 6, 2020
Merged

Layout template #476

merged 11 commits into from
Feb 6, 2020

Conversation

madsi1m
Copy link
Contributor

@madsi1m madsi1m commented Jan 16, 2020

Addresses #473
Requires #469 to be merged first!

@madsi1m madsi1m requested a review from labkode as a code owner January 16, 2020 04:57
@madsi1m
Copy link
Contributor Author

madsi1m commented Jan 16, 2020

Build will break because it needs #469 first

type layoutTemplate struct {
Username string //the username
UsernameLower string //the username in lowercase
FirstLetter string //first letter of username in lowercase
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, FirstLetter limits that dir to a single rune. I'd like to see a variable length to use as the name for the parent dir. I lack the words for a better name of the Parent directory. Prefix does not cut it. Maybe bucket. For s3 storages we were considering the first n chars of an md5 / sha1 of the username to spread users among multiple buckets. But never followed up on it because randomly putting users into buckets seems nice, but in practice you'll end up with unevenly filled buckets when a few powerusers land in the same bucket. Which is why we stored a users home in the account table. That allows moving a single user to a different storage ...

This is better than the current code so still a +1 from me ;-)

Copy link
Contributor Author

@madsi1m madsi1m Jan 21, 2020

Choose a reason for hiding this comment

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

Good idea, added {{.UsernamePrefixCount.x}} which at least solves the ability to set the x to 3 so you can make valid s3 buckets. I'm not sure how to spread users across "buckets" evenly as we cant dictate how a user will use the storage.

@labkode
Copy link
Member

labkode commented Jan 21, 2020

@madsi1m the layout configuration should not go into individual storages like EOS. The path wrapper configuration should suffice to make that happen. Is there any reason why the layout configuration lives also on the eos storage?

@madsi1m
Copy link
Contributor Author

madsi1m commented Jan 21, 2020

Yes, at first I wrote it just for the pw and the eos storage wasn’t using it, for example the createhome doesn’t get the home from the pw. So I changed it to a helper and included it in the storages

@labkode
Copy link
Member

labkode commented Jan 21, 2020

@madsi1m I see so then there is no reason for having the layout on the pw, right?

@madsi1m
Copy link
Contributor Author

madsi1m commented Jan 21, 2020

I guess but I’m not sure what else uses the pw so I put it there also.

Looks like internal/grpc/services/storageprovider/storageprovider.go only uses it.

@labkode Would you like me to commit a change that removes pw from reva?

@labkode
Copy link
Member

labkode commented Jan 22, 2020

@madsi1m if you can remove then I'm happy to merge ;)

@madsi1m
Copy link
Contributor Author

madsi1m commented Jan 23, 2020

done

Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

Looks good

@labkode labkode merged commit 9ac9004 into cs3org:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants