remove redisDB dependency #25

nikooo777 · 2018-07-31T17:43:34Z

This PR aims at removing the local redisdb dependency that makes it impossible to redistribute work across different servers.

Further work has been put in, here's a summary:

Use channel IDs while publishing instead of channel names
Rename API endpoints and associated functions
Fix indexes in information sent to slack (those +1s that you see in the loop)
Slightly changed the way the number of "published" claims is counted (making use of the new synced videos list)
Add 5% slack on UTXOs to avoid waiting a full block even if just a few transactions were made
Speed up startup by eliminating unnecessary block waits
Remove waiting for UTXOs by listing UTXOs (instead we just wait for a new block)
Small bug fixes

lyoshenka · 2018-08-09T14:21:42Z

ytsync/manager.go

@@ -149,7 +165,7 @@ func (s SyncManager) MarkVideoStatus(channelID string, videoID string, status st
 	res, _ := http.PostForm(endpoint, vals)
 	defer res.Body.Close()
 	body, _ := ioutil.ReadAll(res.Body)
-	var response apiSyncUpdateResponse
+	var response apiVideoStatusResponse


you don't have to create a named struct for this if its only used in one place. you can inline it as

var response struct { Success bool `json:"success"` Error null.String `json:"error"` Data null.String `json:"data"` }

ok, will look into that

lyoshenka · 2018-08-09T14:31:56Z

ytsync/ytsync.go

+	lbryChannelID  string
+
+	videosMapMux sync.Mutex
+	mux          sync.Mutex


if you have multiple mutexes, you can't call one of them just mux. its not clear what that's for. it could be something like walletSetupMux. though once you name it that, it becomes clear that something might be wrong with wrapping the whole wallet setup in a single mutex. does the whole thing actually need to be locked?

the mutexes should be pointers, so new copies are not created if the Sync struct is copied

its clearer if you put the mutex right above the thing the mutex is locking, and leave newlines on either side. so in this example, upt videosMapMux right above syncedVideos. and then maybe rename it to syncedVideosMux, like so:

...other vars... syncedVideosMux *sync.Mutex syncedVideos map[string]syncedVideo ...other vars...

you can use a read/write lock to lock the map for reading when you read it, and writing when you write to it. this lets multiple threads read the data at once, which is safe and blocks less. use *sync.RWMutex instead, and call mux.RLock() and mux.RUnlock() when you're only reading from the variable. leave mux.Lock() for writing

Will rename and move the mutexes.
The reason I didn't use read/write locks is that i don't want the application to read when it's being written to, plus the locks are held for a very very short time so there would be no noticeable improvement, only a higher risk of race conditions happening during wallet refills.

a read/write lock does not allow reading during writing. thats the point of every lock. what it does allow is multiple concurrent reads. when you do RLock, others can RLock at the same time. when you Lock (for writing), no one's allowed to Lock or RLock at the same time.

Oh yes, you're right, not sure what was going in my mind. I'll swap that

lyoshenka · 2018-08-09T14:32:30Z

ytsync/ytsync.go

+
+	videosMapMux sync.Mutex
+	mux          sync.Mutex
+	wg           sync.WaitGroup


this should also be a pointer. but more importantly, you don't need this if you already have a stop.Group. stop.Group works as a combo WaitGroup + channel that will be closed to indicate stopping (and can be closed safely multiple times). so use grp.Add() and grp.Wait() instead

it won't let me comment below, so I'm commenting here:

you call Add() and Done() inside startWorker(workerNum int), but the correct pattern is to make those calls outside the function. startWorker() doesn't know if its being run asynchronously or not. you only need a waitgroup if it is. there are also subtle concurrency issues with calling it inside the function. so the right way to go is to remove Add and Done from inside startWorker, and do this:

for i := 0; i < s.ConcurrentVideos; i++ { s.wg.Add(1) go func() { defer s.wg.Done() s.startWorker(i) }() }

I need to look into that, this is from your original code and I don't remember ever changing it. Thanks for the pointers there.

I looked more into why we need to lock the whole walletSetup function. The reason I'm doing that is to avoid multiple threads from refilling the wallet concurrently. I don't think I can easily break up the the function to lock fewer lines of code. I think it's fair to leave it like that.

I removed the wait group in favor of the stop group

lyoshenka · 2018-08-09T14:42:20Z

ytsync/ytsync.go

@@ -182,6 +195,9 @@ func (s *Sync) FullCycle() (e error) {
 	}

 	s.db = redisdb.New()
+	s.videosMapMux.Lock()


why is this code all the way down here, so far from line 125 where syncedVideos is first declared?

the only reason i put it there is because there were a bunch of assignments there and i wanted to keep them grouped, but I agree it's not very read-able this way. I'll move them up

lyoshenka · 2018-08-09T14:47:21Z

ytsync/setup.go

-	}
+	s.videosMapMux.Lock()
+	numPublished := uint64(len(s.syncedVideos)) //should we only count published videos? Credits are allocated even for failed ones...
+	s.videosMapMux.Unlock()
 	log.Debugf("We already published %d videos", numPublished)

 	if float64(numOnSource)-float64(numPublished) > float64(s.Manager.VideosLimit) {


why are all these cast to floats? they can be ints here

no clue. but you're completely right. Will fix

lyoshenka · 2018-08-09T14:55:30Z

ytsync/ytsync.go

+		"the video is too big to sync, skipping for now",
+	}
+	if ok && !sv.Published && util.SubstringInSlice(sv.FailureReason, neverRetryFailures) {
+		log.Println(v.ID() + " can't ever be published")


how often does this happen, and do you know why? is it within our control?

the first happens due to youtube restrictions (either region, age or deleted videos)
the second happens when we hit the defined size limit (it's in our control).

While we can't do anything about the first, we can raise the limits for the second, but as per several discussions we'll keep it at 2GB or less.

out of 100k published videos we have about 1% that failed for those two (leading) reasons: https://scrn.storni.info/2018-08-09_12-24-24-339707393.png

use video statuses

reinstate redis db to detect inconsistencies

add locking fix bugs

should make startup faster bug fixes

bad bug fixes remove half broken code used to wait for UTXOs use channel IDs rather than names (skip lbryum) port changes to UCB uploader

refactoring address reviews

nikooo777 · 2018-08-14T15:21:30Z

addressed reviews

tiger5226 · 2018-08-16T22:38:15Z

ytsync/manager.go

@@ -149,7 +159,11 @@ func (s SyncManager) MarkVideoStatus(channelID string, videoID string, status st
 	res, _ := http.PostForm(endpoint, vals)
 	defer res.Body.Close()
 	body, _ := ioutil.ReadAll(res.Body)
-	var response apiSyncUpdateResponse
+	var response struct {


why are we not using the aligned code in the api package? I would make sure we are making things DRY. api.Response

tiger5226 · 2018-08-16T23:05:32Z

ytsync/setup.go

-	s.mux.Lock()
-	defer s.mux.Unlock()
+	s.walletMux.Lock()
+	defer s.walletMux.Unlock()


why are we using a mutex lock? What is wrong with concurrent execution? A mutex is not the standard way to handle synchronization. It can cause deadlocks. Ideally if you want things to run synchronously from different go routines, you use channels in golang.

this is the only place the lock is used and no multiple locks are being held that can cause a deadlock here.
However I would like to discuss channels with you to understand how they could be used here (not sure they can)

tiger5226 · 2018-08-16T23:06:12Z

ytsync/setup.go

@@ -30,31 +30,32 @@ func (s *Sync) walletSetup() error {
 	balance := decimal.Decimal(*balanceResp)
 	log.Debugf("Starting balance is %s", balance.String())

-	var numOnSource uint64
+	var numOnSource int


why change this to an int? It better to be more specific than more general.

Grins previous review outlined a mess with casts here and there to make simple math.
I changed everything to int as it's reasonable for the values they represent.

tiger5226 · 2018-08-16T23:07:02Z

ytsync/setup.go

@@ -30,31 +30,32 @@ func (s *Sync) walletSetup() error {
 	balance := decimal.Decimal(*balanceResp)
 	log.Debugf("Starting balance is %s", balance.String())

-	var numOnSource uint64
+	var numOnSource int
 	if s.LbryChannelName == "@UCBerkeley" {
 		numOnSource = 10104


what is this?! not part of the PR but this is not good to have in the code base.

I can explain the berkeley stuff to you via DM, it's all good as it will be eventually removed from here. Not worth changing now

tiger5226 · 2018-08-16T23:10:58Z

ytsync/setup.go

+		return err
+	}
+
+	for status.Wallet.Blocks == 0 || status.Wallet.BlocksBehind != 0 {


so is this ok? If there is a problem it would be an infinite loop no? Maybe have a maxit?

tiger5226 · 2018-08-16T23:13:22Z

ytsync/sources/ucbVideo.go

-	if channelName != "" {
-		options.ChannelName = &channelName
+		Title:         &v.title,
+		Author:        strPtr("UC Berkeley"),


Why is the author UC Berkley? should this function be called publishUCBerkley?

ahhh, ucbVideo Seems odd to have a struct type for a specific author.

tiger5226 · 2018-08-16T23:21:55Z

ytsync/ytsync.go

-	queue chan video
+func (s *Sync) AppendSyncedVideo(videoID string, published bool, failureReason string) {
+	s.syncedVideosMux.Lock()
+	defer s.syncedVideosMux.Unlock()


I don't understand why we have a mutex lock here. Why would this get called twice in different go routines on the same sync instance?

videos end concurrently, that's why

tiger5226 · 2018-08-16T23:23:54Z

ytsync/ytsync.go

+	interruptChan := make(chan os.Signal, 1)
+	signal.Notify(interruptChan, os.Interrupt, syscall.SIGTERM)
+	go func() {
+		<-interruptChan


we should not do this here. Please use the stopper pattern grin built in the stop package.

tiger5226 · 2018-08-16T23:25:01Z

ytsync/ytsync.go

 	if err != nil {
 		return err
 	}
+	s.syncedVideosMux.Lock()
+	s.syncedVideos = syncedVideos
+	s.syncedVideosMux.Unlock()


Last comment on mutex locks. We should discuss the pattern you are using here, as I am almost positive using channels is a better and more robust solution design.

lyoshenka

i agree with most of beamer's comments, and none are major blockers, so im fine with merging this now and cleaning up as you continue to work with the code.

in general im trying to be less strict on PRs when the code works but has little nits in it.

lbry-bot assigned nikooo777 Jul 31, 2018

nikooo777 force-pushed the use_video_statuses branch 3 times, most recently from d83aebc to 595cd27 Compare August 3, 2018 18:09

nikooo777 requested a review from lyoshenka August 8, 2018 22:16

lbry-bot assigned lyoshenka and unassigned nikooo777 Aug 8, 2018

lyoshenka requested changes Aug 9, 2018

View reviewed changes

lbry-bot assigned nikooo777 and unassigned lyoshenka and nikooo777 Aug 9, 2018

nikooo777 added 8 commits August 9, 2018 20:13

remove redis dependency

c428b5e

use video statuses

refactor and fix api response handlers

b3f7f93

reinstate redis db to detect inconsistencies

fix oversights

630ca87

update synced videos map on progress

3956e0d

add locking fix bugs

change utxos confirmation behavior

9cfda0d

should make startup faster bug fixes

add function to wait for a new block

3a51ea9

bad bug fixes remove half broken code used to wait for UTXOs use channel IDs rather than names (skip lbryum) port changes to UCB uploader

fix mysterious bug

688ce0d

small bug fixes

37c1595

nikooo777 force-pushed the use_video_statuses branch from 798e7cc to 37c1595 Compare August 10, 2018 00:14

bug fixes

58c8e7f

nikooo777 force-pushed the use_video_statuses branch from 101bda3 to 58c8e7f Compare August 10, 2018 00:20

nikooo777 added 2 commits August 10, 2018 08:41

fix nastiest bug ever

2cb9082

use stop group instead of wait group

38c1883

refactoring address reviews

nikooo777 requested a review from lyoshenka August 14, 2018 15:21

lbry-bot assigned lyoshenka Aug 14, 2018

This was referenced Aug 14, 2018

lbrynet/lbrycrd wallet management #34

Merged

Better tracking (Size and failure reason) #35

Merged

tiger5226 reviewed Aug 16, 2018

View reviewed changes

tiger5226 requested changes Aug 16, 2018

View reviewed changes

don't publish when running out of space

86adadb

lbry-bot assigned lyoshenka and unassigned lyoshenka Aug 18, 2018

lyoshenka approved these changes Aug 18, 2018

View reviewed changes

lbry-bot assigned nikooo777 and unassigned nikooo777 Aug 18, 2018

nikooo777 mentioned this pull request Aug 20, 2018

Address old review comments #31

Closed

11 tasks

nikooo777 merged commit 1d31c57 into master Aug 20, 2018

nikooo777 deleted the use_video_statuses branch August 20, 2018 11:59

This was referenced Nov 1, 2018

Address old review comments lbryio/ytsync#4

Closed

Address old review comments lbryio/ytsync#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove redisDB dependency #25

remove redisDB dependency #25

nikooo777 commented Jul 31, 2018 •

edited

Loading

lyoshenka Aug 9, 2018

nikooo777 Aug 9, 2018

lyoshenka Aug 9, 2018

lyoshenka Aug 9, 2018

nikooo777 Aug 9, 2018

lyoshenka Aug 18, 2018

nikooo777 Aug 20, 2018

lyoshenka Aug 9, 2018

lyoshenka Aug 9, 2018

nikooo777 Aug 9, 2018

nikooo777 Aug 14, 2018

lyoshenka Aug 9, 2018

nikooo777 Aug 9, 2018

lyoshenka Aug 9, 2018

nikooo777 Aug 9, 2018

lyoshenka Aug 9, 2018

nikooo777 Aug 9, 2018

nikooo777 Aug 9, 2018

nikooo777 commented Aug 14, 2018

tiger5226 Aug 16, 2018

tiger5226 Aug 16, 2018

nikooo777 Aug 20, 2018

tiger5226 Aug 16, 2018

nikooo777 Aug 20, 2018

tiger5226 Aug 16, 2018

nikooo777 Aug 20, 2018

tiger5226 Aug 16, 2018

tiger5226 Aug 16, 2018

tiger5226 Aug 16, 2018

tiger5226 Aug 16, 2018

nikooo777 Aug 20, 2018

tiger5226 Aug 16, 2018

tiger5226 Aug 16, 2018

lyoshenka left a comment

remove redisDB dependency #25

remove redisDB dependency #25

Conversation

nikooo777 commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikooo777 commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyoshenka left a comment

Choose a reason for hiding this comment

nikooo777 commented Jul 31, 2018 •

edited

Loading