Better tracking (Size and failure reason) #35

2018-08-14T17:29:25+02:00

nikooo777 commented

2018-08-14 17:29:25 +02:00

(Migrated from github.com)

WiP
This PR can only be reviewed/merged after #25 and #34 are merged
also it depends on https://github.com/lbryio/internal-apis/pull/522

WiP This PR can only be reviewed/merged after #25 and #34 are merged also it depends on https://github.com/lbryio/internal-apis/pull/522

lyoshenka (Migrated from github.com) requested changes 2018-08-27 20:02:24 +02:00

lyoshenka (Migrated from github.com) left a comment

passing around the mutex to all these functions is a bad pattern. if the mutex is protecting something, you should create a new struct and put the mutex and the thing into it. then you just pass the struct around, and the two are always together.

ytsync/sources/ucbVideo.go

					
				@ -40,19 +44,19 @@ func NewUCBVideo(id, title, channel, description, publishedAt, dir string) ucbVi

					}

				}

lyoshenka (Migrated from github.com) commented

2018-08-27 19:59:17 +02:00

why are these fields here if they never get used?

lyoshenka (Migrated from github.com) commented

2018-08-27 20:00:08 +02:00

and having syncedVideosMux on each video is weird. whats going on?

ytsync/ytsync.go

					
				@ -373,3 +443,3 @@

						s.grp.Add(1)

						go func() {

						go func(i int) {

							defer s.grp.Done()

lyoshenka (Migrated from github.com) commented

2018-08-27 20:01:24 +02:00

why does this function return a pointer to an error?

nikooo777 (Migrated from github.com) reviewed 2018-08-27 20:11:34 +02:00

ytsync/ytsync.go

					
				@ -373,3 +443,3 @@

						s.grp.Add(1)

						go func() {

						go func(i int) {

							defer s.grp.Done()

nikooo777 (Migrated from github.com) commented

2018-08-27 20:11:34 +02:00

because the function is run as a deferred and needs to read/write to the error being returned by the original function. Is this a bad pattern?

nikooo777 (Migrated from github.com) reviewed 2018-08-27 20:12:54 +02:00

ytsync/sources/ucbVideo.go

					
				@ -40,19 +44,19 @@ func NewUCBVideo(id, title, channel, description, publishedAt, dir string) ucbVi

					}

				}

nikooo777 (Migrated from github.com) commented

2018-08-27 20:12:54 +02:00

there is only one mutex for the whole map, the pattern is the same you used locally in the function where a name is generated for the claim.
I however agree with your comment about passing down the mutex, I'm going to correct that.

there is only one mutex for the whole map, the pattern is the same you used locally in the function where a name is generated for the claim. I however agree with your comment about passing down the mutex, I'm going to correct that.

nikooo777 (Migrated from github.com) reviewed 2018-08-27 20:18:36 +02:00

ytsync/ytsync.go

					
				@ -373,3 +443,3 @@

						s.grp.Add(1)

						go func() {

						go func(i int) {

							defer s.grp.Done()

nikooo777 (Migrated from github.com) commented

2018-08-27 20:18:36 +02:00

well actually to be correct it's not returning a pointer to an error, it's accepting a pointer to an error so that I can change it from within.

Sign in to join this conversation.

No reviewers

lyoshenka

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry.go#35