Fixes issue #3558 Initialize cache after DCP start #3607

tleyden · 2018-05-25T23:05:29Z

Summary of changes:

Get current (latest) global doc sequence after starting DCP feed, to eliminate chance of race
Locks change cache for a brief period of time while the global doc sequence is looked up, during which time any incoming DCP events should have processing blocked
Rename onChange -> notifyChange
There was an error being ignored in the Clear() method -- this now propagates the error to callers.

Integration tests

http://uberjenkins.sc.couchbase.com/view/Build/job/sync-gateway-integration-master/725/
http://uberjenkins.sc.couchbase.com/view/Build/job/sync-gateway-integration-master/726/

tleyden · 2018-05-26T16:07:06Z

Ready for a review pass -- builds passing

adamcfraser · 2018-05-29T20:30:44Z

db/change_cache.go

@@ -169,14 +175,25 @@ func (c *changeCache) IsStopped() bool {
 	return c.stopped
 }

-// Forgets all cached changes for all channels.
-func (c *changeCache) Clear() {
+// Empty out all channel caches.  This doesn't touch


Comment looks like it was truncated

adamcfraser · 2018-05-30T16:52:47Z

db/change_cache.go

+//
+// - Manage collection of channel caches
+// - Receive DCP changes via callbacks
+//    - Perform sequence buffering to handle skipped sequences


I think this comment is misleading - sequence buffering is primarily about getting documents in sequence order. Skipped sequence handling is a separate concern.

adamcfraser · 2018-05-30T16:57:33Z

db/change_cache.go

-	nextSequence    uint64                   // Next consecutive sequence number to add
-	initialSequence uint64                   // DB's current sequence at startup time
+	nextSequence    uint64                   // Next consecutive sequence number to add.  State variable for sequence buffering tracking.  Should use getNextSequence() rather than accessing directly.
+	initialSequence uint64                   // DB's current sequence at startup time. Should use getInititalSequence() rather than accessing directly.


Typo in comment

My editor isn't flagging anything .. nor is it jumping out at me.

Fixed getInitialSequence()

adamcfraser · 2018-05-30T16:58:14Z

db/change_cache.go

+}
+
+// Set the initial sequence.
+// Think of this as _SetInitialSequence() -- since it premumes that change chache is locked.  It is not safe to


Typo in comment

adamcfraser · 2018-05-30T16:59:09Z

db/change_cache.go

 	if event.Synchronous {
 		c.DocChangedSynchronous(event)
 	} else {
 		go c.DocChangedSynchronous(event)
 	}
 }

+// Set's the initial sequence, assume's that the caller is already holding a lock on the changeCache
+func (c *changeCache) _setInitialSequence(initialSequence uint64) {


How does this differ from SetInitialSequence below?

Removed extraneous

adamcfraser · 2018-05-30T17:03:00Z

db/database.go

+		// Find the current global doc sequence and use that for the initial sequence for the change cache
+		lastSequence, err := context.LastSequence()
+		if err != nil {
+			return nil, err


Need to unlock the changeCache on error here as well?

Good catch. Fixed.

adamcfraser · 2018-05-30T17:09:59Z

db/database.go

+
+		if err != nil {
+			// In the case of an error, unlock the change cache despite the fact it's in an unitialized state.
+			context.changeCache.StartupUnlock()


Looking at this again, I'm still not thrilled about trying to manage the changeCache's internal lock from an external location. Would this approach work:

changeCache.Init() leaves the cache locked (not started)

A subsequent call to changeCache.Start() does the following:

retrieves and sets initial/next sequence

unlocks the cache
This would also let us push everything from line 313 to 324 down into changeCache.Start(), which I think makes more sense (it's cache-related code).

Yeah, agreed that it's awkward to have the external locking semantics. Fixed in 8ee61c5

This reverts commit bd5e502.

…feed

…LazyLoaded == false

WARNING: DATA RACE Write at 0x00c4201fa2e0 by goroutine 9: github.com/couchbase/sync_gateway/db.(*changeCache)._addToCache() /drone/godeps/src/github.com/couchbase/sync_gateway/db/change_cache.go:689 +0x4ff github.com/couchbase/sync_gateway/db.(*changeCache).processEntry() /drone/godeps/src/github.com/couchbase/sync_gateway/db/change_cache.go:650 +0x1f6 github.com/couchbase/sync_gateway/db.(*changeCache).DocChangedSynchronous() /drone/godeps/src/github.com/couchbase/sync_gateway/db/change_cache.go:549 +0x20a4 Previous read at 0x00c4201fa2e0 by goroutine 42: github.com/couchbase/sync_gateway/db.(*changeCache).waitForSequence() /drone/godeps/src/github.com/couchbase/sync_gateway/db/util_testing.go:25 +0xbd github.com/couchbase/sync_gateway/db.TestDocDeletionFromChannelCoalesced() /drone/godeps/src/github.com/couchbase/sync_gateway/db/changes_test.go:233 +0x5fe testing.tRunner() /usr/local/go/src/testing/testing.go:746 +0x16c

https://gist.github.com/tleyden/dccaa3cf2be85160051a793333c24467

tleyden · 2018-05-30T18:36:50Z

Rebased on dev and kicked off a round of integration tests:

http://uberjenkins.sc.couchbase.com/view/Build/job/sync-gateway-integration-master/725/
http://uberjenkins.sc.couchbase.com/view/Build/job/sync-gateway-integration-master/726/

adamcfraser · 2018-05-30T20:33:48Z

LGTM - can merge if integration tests are clean.

* Add experimental dockerfile * Fix attempt for build err * Trying to figure out how to get source commit/branch * Revert "Trying to figure out how to get source commit/branch" This reverts commit bd5e502. * Add artificial delay after getting last seq, but before starting dcp feed * Reload c.initialSequence in DocChanged() as long as c.initialSequenceLazyLoaded == false * Notes from call w/ Adam * Add c.getInitialSequence() + _getNextSequence() * Fixes * Add comments * Notes from call w/ Adam * Sketch out changes and test * Change the changeindex iface * Fix data race WARNING: DATA RACE Write at 0x00c4201fa2e0 by goroutine 9: github.com/couchbase/sync_gateway/db.(*changeCache)._addToCache() /drone/godeps/src/github.com/couchbase/sync_gateway/db/change_cache.go:689 +0x4ff github.com/couchbase/sync_gateway/db.(*changeCache).processEntry() /drone/godeps/src/github.com/couchbase/sync_gateway/db/change_cache.go:650 +0x1f6 github.com/couchbase/sync_gateway/db.(*changeCache).DocChangedSynchronous() /drone/godeps/src/github.com/couchbase/sync_gateway/db/change_cache.go:549 +0x20a4 Previous read at 0x00c4201fa2e0 by goroutine 42: github.com/couchbase/sync_gateway/db.(*changeCache).waitForSequence() /drone/godeps/src/github.com/couchbase/sync_gateway/db/util_testing.go:25 +0xbd github.com/couchbase/sync_gateway/db.TestDocDeletionFromChannelCoalesced() /drone/godeps/src/github.com/couchbase/sync_gateway/db/changes_test.go:233 +0x5fe testing.tRunner() /usr/local/go/src/testing/testing.go:746 +0x16c * Use getNextSequence() in a few more places * Fix another race https://gist.github.com/tleyden/dccaa3cf2be85160051a793333c24467 * Remove dockerfile, moved to a separate PR * Gofmt * PR cleanup * PR cleanup * Remove check that’s leftover from first attempt * Add comments * First round of PR feedback * Address PR feedback about avoiding external locking

tleyden added in progress review labels May 25, 2018

tleyden force-pushed the feature/issue_3558_int_cache_post_dcp branch from eb5c138 to ecd7fe7 Compare May 26, 2018 03:11

tleyden requested a review from adamcfraser May 26, 2018 16:06

tleyden assigned adamcfraser May 26, 2018

adamcfraser requested changes May 30, 2018

View reviewed changes

tleyden assigned adamcfraser and unassigned adamcfraser May 30, 2018

Traun Leyden added 21 commits May 30, 2018 11:35

Add experimental dockerfile

f4dca7b

Fix attempt for build err

6d0c17c

Trying to figure out how to get source commit/branch

9421e36

Revert "Trying to figure out how to get source commit/branch"

47815b6

This reverts commit bd5e502.

Add artificial delay after getting last seq, but before starting dcp …

8269e56

…feed

Reload c.initialSequence in DocChanged() as long as c.initialSequence…

6795546

…LazyLoaded == false

Notes from call w/ Adam

d2c82df

Add c.getInitialSequence() + _getNextSequence()

696cb0e

Fixes

5dfd9a6

Add comments

48bbb85

Notes from call w/ Adam

0c1eecd

Sketch out changes and test

2790336

Change the changeindex iface

c69aa73

Use getNextSequence() in a few more places

19ca5a6

Fix another race

0f7f25c

https://gist.github.com/tleyden/dccaa3cf2be85160051a793333c24467

Remove dockerfile, moved to a separate PR

c454533

Gofmt

5393590

PR cleanup

64c8d7e

PR cleanup

1e50db4

Remove check that’s leftover from first attempt

52f2903

Traun Leyden added 3 commits May 30, 2018 11:35

Add comments

afb500a

First round of PR feedback

5e884e6

Address PR feedback about avoiding external locking

0be977f

tleyden force-pushed the feature/issue_3558_int_cache_post_dcp branch from 8ee61c5 to 0be977f Compare May 30, 2018 18:35

adamcfraser approved these changes May 30, 2018

View reviewed changes

tleyden merged commit b55a593 into dev May 30, 2018

tleyden deleted the feature/issue_3558_int_cache_post_dcp branch May 30, 2018 21:40

tleyden mentioned this pull request May 31, 2018

Initialize cache after DCP start #3558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue #3558 Initialize cache after DCP start #3607

Fixes issue #3558 Initialize cache after DCP start #3607

tleyden commented May 25, 2018 •

edited

tleyden commented May 26, 2018

adamcfraser May 29, 2018

tleyden May 30, 2018

adamcfraser May 30, 2018

tleyden May 30, 2018

adamcfraser May 30, 2018

tleyden May 30, 2018

tleyden May 30, 2018

adamcfraser May 30, 2018

tleyden May 30, 2018

adamcfraser May 30, 2018

tleyden May 30, 2018

adamcfraser May 30, 2018

tleyden May 30, 2018

adamcfraser May 30, 2018

tleyden May 30, 2018

tleyden commented May 30, 2018

adamcfraser commented May 30, 2018

Fixes issue #3558 Initialize cache after DCP start #3607

Fixes issue #3558 Initialize cache after DCP start #3607

Conversation

tleyden commented May 25, 2018 • edited

Integration tests

tleyden commented May 26, 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

Choose a reason for hiding this comment

tleyden commented May 30, 2018

adamcfraser commented May 30, 2018

tleyden commented May 25, 2018 •

edited