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

Implement login/logout, registry, bandwidth used. #33

Merged
merged 27 commits into from Feb 23, 2021
Merged

Conversation

ro-tex
Copy link
Contributor

@ro-tex ro-tex commented Feb 17, 2021

Features:

  • login/logout: endpoints for setting and removing the skynet-jwt cookie
  • tracking registry reads and writes and count those towards used bandwidth
  • tracking used bandwidth
  • support streaming downloads - we decided to treat them as separate downloads for the moment

@ro-tex ro-tex self-assigned this Feb 17, 2021
@ro-tex ro-tex marked this pull request as draft February 17, 2021 17:20
@ro-tex ro-tex marked this pull request as ready for review February 18, 2021 17:44
api/handlers.go Outdated Show resolved Hide resolved
api/handlers.go Outdated Show resolved Hide resolved
test/user_test.go Outdated Show resolved Hide resolved
test/upload_test.go Outdated Show resolved Hide resolved
@ro-tex ro-tex requested a review from mrcnski February 19, 2021 10:27
mrcnski
mrcnski previously approved these changes Feb 22, 2021
api/cookie.go Show resolved Hide resolved
api/handlers.go Outdated Show resolved Hide resolved
api/handlers.go Outdated Show resolved Hide resolved
@@ -23,7 +24,7 @@ type UploadResponseDTO struct {
ID string `bson:"_id" json:"id"`
Skylink string `bson:"skylink" json:"skylink"`
Name string `bson:"name" json:"name"`
Size uint64 `bson:"size" json:"size"`
Size int64 `bson:"size" json:"size"`

Choose a reason for hiding this comment

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

any reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's confusing to keep converting between int64 and uint64, so I started using int64 for all size and bandwidth values.

database/upload.go Show resolved Hide resolved

// userBandwidth reports the total bandwidth used by the user.
func (db *DB) userBandwidth(ctx context.Context, id primitive.ObjectID) (int64, error) {
var bandwidthAtomic int64

Choose a reason for hiding this comment

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

why not uint64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency. All file sizes and bandwidth are now int64. Is there a benefit of having this be uint64?

test/user_test.go Show resolved Hide resolved
{size: 1 * MiB, result: 40 * MiB},
{size: 4 * MiB, result: 40 * MiB},
{size: 5 * MiB, result: (40 + 120) * MiB},
{size: 50 * MiB, result: (40 + 2*120) * MiB},

Choose a reason for hiding this comment

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

I'm not sure I'm following here, could you elaborate on why a 50MiB upload results in 290MiB bandwidth usage/cost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base sector is 4MB and has 1/10 redundancy, so it uses 40MB.
The remaining 46MB fit into 2 chunks of 40MB each. Those have 1/3 redundancy, so they use 120MB per chunk.

40 + 2*120 = 280

database/user.go Outdated
return time.Time{}, errors.AddContext(err, "failed to fetch user")
}
now := time.Now().UTC()
daysDelta := now.Day() - user.SubscribedUntil.Day()

Choose a reason for hiding this comment

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

Could you explain in a comment what the goal is here, it's confusing, at least to me.
So you take the subscribed until, and subtract that from the current day, so if we're
feb 3rd, and he was subscribed until jan 28th, that's -25. Then you add 25 days to today,
I guess that's feb 28 or mar 1, and daysDelta was smaller than zero, so then you subtract a month and use daysDelta is the day? Surely there is a bug in this piece of code right, I don't see it but I'm pretty sure there is one :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Time is hard. :D

Yes, there was a bug. I hope I managed to fix it. I also added a lengthy comment.

Here is how it looks now:

func MonthStart(subscribedUntil time.Time) time.Time {
	now := time.Now().UTC()
	// Check how many days are left until the end of the user's subscription
	// month. Then calculate when the last subscription month started. We don't
	// care if the user is no longer subscribed and their sub expired 3 months
	// ago, all we care about here is the day of the month on which that
	// happened because that is the day from which we count their statistics for
	// the month. If they were never subscribed we use Jan 1st 1970 for
	// SubscribedUntil.
	daysDelta := subscribedUntil.Day() - now.Day()
	d := now.AddDate(0, -1, daysDelta)
	return time.Date(d.Year(), d.Month(), d.Day(), 0, 0, 0, 0, time.UTC)
}

database/user.go Outdated Show resolved Hide resolved
@peterjan
Copy link

Overall it looks good, keeping in mind this is a beta release though. I went over it but decided not to nit nor go into much detail. I guess any bugs will surface and are non-vital at the moment. We've cut quite a few corners so I guess the important thing is to get it out.

peterjan
peterjan previously approved these changes Feb 23, 2021
@ro-tex ro-tex dismissed stale reviews from peterjan and mrcnski via ecc001d February 23, 2021 12:35
peterjan
peterjan previously approved these changes Feb 23, 2021
database/user_test.go Outdated Show resolved Hide resolved
mrcnski
mrcnski previously approved these changes Feb 23, 2021
@ro-tex ro-tex dismissed stale reviews from mrcnski and peterjan via cd01adb February 23, 2021 14:46
@ro-tex ro-tex merged commit bbf5713 into main Feb 23, 2021
@ro-tex ro-tex deleted the ivo/login_logout branch February 23, 2021 14:56
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.

None yet

3 participants