ep-engine.git
2 years ago[BP] MB-29531: Replace revSeqno with a 48-bit counter 00/93800/4 watson v4.6.5
Jim Walker [Sun, 6 May 2018 19:29:12 +0000 (20:29 +0100)]
[BP] MB-29531: Replace revSeqno with a 48-bit counter

Prevent a value too large to be stored in couchstore
from being placed into Item/StoredValue and also the
_local document (via vbucket_state).

Change-Id: I6de783dc2374c2634f1a729e4ca5fa2bc35dda40
Reviewed-on: http://review.couchbase.org/93800
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
2 years agoMB-29483: Disable DCP cursor dropping 34/93534/4
Dave Rigby [Tue, 1 May 2018 10:25:21 +0000 (11:25 +0100)]
MB-29483: Disable DCP cursor dropping

Disable DCP cursor dropping:

1. DCP replication consumers no longer request cursor dropping.
2. DCP producers default cursorDropping to false, and ignore any
   requests to enable it.

Change-Id: Ia9b5620d43c821eeaa32a7ec67df419a51acc089
Reviewed-on: http://review.couchbase.org/93534
Reviewed-by: Tim Bradgate <tim.bradgate@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
2 years agoMB-29287: Add dcp_reflection_test 84/92984/9
Dave Rigby [Thu, 19 Apr 2018 13:19:35 +0000 (14:19 +0100)]
MB-29287: Add dcp_reflection_test

Add new set of DCP unit tests, which connect DcpProducer and DcpConsumer
objects; and reflect the messages between them. This allows us to test
how the producer and consumer communicate; without involving
ns_server.

The tests are purely constrained to ep-engine - i.e. no packets are
actually transmitted over the network; we instead just inject the
messages directly into the DcpProducer / DcpConsumer objects.

Change-Id: I641826238dc09c9b94d6540b8e4a5edc656883dc
Reviewed-on: http://review.couchbase.org/92984
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Tim Bradgate <tim.bradgate@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
2 years agoMB-29287: Move SynchronousEPEngine building to seperate method 43/93043/6
Dave Rigby [Fri, 20 Apr 2018 08:13:33 +0000 (09:13 +0100)]
MB-29287: Move SynchronousEPEngine building to seperate method

Refactor the construction and setup of SynchronousEPEngine from
EventuallyPersistentStoreTest::SetUp() to a seperate builder function
- SynchronousEPEngine::build().

This allows tests which want to instantiate more than one ep-engine
instance to do so.

Change-Id: I647d32a28a2c3a1e1bfbca563c2a1ebe5130ad56
Reviewed-on: http://review.couchbase.org/93043
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
2 years agoMB-29287: Add string / streaming methods for DCP responses 83/92983/6
Dave Rigby [Thu, 19 Apr 2018 13:14:18 +0000 (14:14 +0100)]
MB-29287: Add string / streaming methods for DCP responses

Allow printing of DCP response codes in log messages / test code.

Change-Id: I7d48625fbf3efcad1cfb916686f585eec42e2b46
Reviewed-on: http://review.couchbase.org/92983
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Tim Bradgate <tim.bradgate@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
2 years agoMB-29287: Avoid ambiguity in ActiveStream cursor name 07/93407/2
Dave Rigby [Fri, 27 Apr 2018 08:37:38 +0000 (09:37 +0100)]
MB-29287: Avoid ambiguity in ActiveStream cursor name

Handle the possible edge-case where the user-provided node-name plus
counter could be ambiguous - from Aliaksey:

    Consider the following case. Lets say we replicate to two nodes
    here, one named 'node' the other one 'node1'. We create
    replication to node1 and the counter is 0. Then after some time
    replication to node is created when the counter is 10. Now you
    have two totally different replications using the same cursor.

    ns_server happens to put the bucket name in the end of the
    connection name. So it saves us from the case I described. But a
    malicious client could craft a name that causes this sort of
    conflict.

This is very unlikely to occur, but not impossible; so for the sake of
being completeness avoid this situation by inserting a fixed seperator
between the user-provided name and the counter.

Change-Id: Ie308a09d420d8275ed059e834c09684675f9f737
Reviewed-on: http://review.couchbase.org/93407
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Tim Bradgate <tim.bradgate@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
2 years agoMB-29381: Allow DCP rollback on vbuckets in pending state 10/93110/4
Manu Dhundi [Mon, 23 Apr 2018 14:15:16 +0000 (15:15 +0100)]
MB-29381: Allow DCP rollback on vbuckets in pending state

We currently allow rollbacks on only replica vbuckets. This was
added recently (on 4.6.0 with http://review.couchbase.org/#/c/69725).

But during vbucket move, the new master is initially created in
pending state, items streamed over from old master and finally a
takeover stream is created. If there is a rollback during the
takeover, then we should still allow it on the vbucket which in the
pending state.

(Backport of MB-26037 / commit
496d9b96cd046441440022406d561dbc2bd043b5).

Change-Id: Ie57aed02d574be7b8a3852da5a948ef688676770
Reviewed-on: http://review.couchbase.org/93110
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Paolo Cocchi <paolo.cocchi@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: Build Bot <build@couchbase.com>
2 years agoMB-29287: Test that takeover finds all items 23/93123/3
Jim Walker [Fri, 20 Apr 2018 14:30:45 +0000 (15:30 +0100)]
MB-29287: Test that takeover finds all items

Using the single threaded test harness we can demonstrate the window
in which a close-stream and create stream will lose items. By
triggering the close/re-create from within the snapshot processor
after it has a handle on the stream which will be closed and before
it access the items, we will transfer items into the dead stream
and prevent the new takeover stream from obtaining them.

Change-Id: I896a7cbd3d32419576294ea88c288b097759a362
Reviewed-on: http://review.couchbase.org/93123
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
2 years agoMB-29287: Give each ActiveStream a unique cursor name 12/93112/2
Jim Walker [Fri, 20 Apr 2018 09:24:37 +0000 (10:24 +0100)]
MB-29287: Give each ActiveStream a unique cursor name

To ensure that when the background snapshot processor task runs
and we have closed/created the ActiveStream, if the task gets a handle
on the closed stream we must be sure it does not obtain items destined
for the new stream. Previously with each ActiveStream just using the
name of its producer, the closed stream was able to drain items which
the new stream needed.

Adding a monotonic atomic to the ActiveStream class and appending a
new value to the name we use for the cursor ensures concurrent streams
cannot interfere with each other.

Change-Id: Ie05092490a75c656c344425850eba00043019e96
Reviewed-on: http://review.couchbase.org/93112
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years agoMB-26979: ChkptProcessorTask should not own dcp stream objs 77/86277/8
Manu Dhundi [Thu, 7 Dec 2017 21:47:16 +0000 (13:47 -0800)]
MB-26979: ChkptProcessorTask should not own dcp stream objs

Currently the ActiveStreamCheckpointProcessorTask co-owns the
corresponding stream object. So if the producer connection
(co-owner) quickly closes the stream and opens a new stream on the same
vbucket (that is the streams map of the producer will contain
new stream object), then the ActiveStreamCheckpointProcessorTask
will contain any entry to the older stream. This could result in a
case where the processor task is not run for the new stream which
could result in a DCP hang.

This commit fixes the issue by making sure that the processor
task only contains the vbucket id and the stream is looked up from
the producer streams map when needed. However this requires the
ActiveStreamCheckpointProcessorTask to hold a shared reference to
the producer though the producer obj holds a shared reference to
the task (thereby resulting in a cyclic reference). Hence in the delete
path, the cyclic reference is broken by manually deleting the producer
reference.

The problem of cyclic reference can be averted in the master branch
by the use of shared/weak ptr.

Also, to test the code, some refactoring of test code is done.

Change-Id: I4b16bb81aac6f45a137affa9870be6f1416e9464
Reviewed-on: http://review.couchbase.org/86277
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
3 years agoMB-24142: Use correct unit for slowTask recording 24/84424/3 4.6.4 v4.6.4
Dave Rigby [Thu, 27 Apr 2017 15:18:53 +0000 (16:18 +0100)]
MB-24142: Use correct unit for slowTask recording

Change-Id: I388eb43fea01f8b79f5a122afa2c68757736fb81
Reviewed-on: http://review.couchbase.org/84424
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
3 years ago[BP] MB-25798: Don't schedule backfill until previous is complete 33/83533/2
Daniel Owen [Wed, 29 Mar 2017 14:58:09 +0000 (15:58 +0100)]
[BP] MB-25798: Don't schedule backfill until previous is complete

Previously in the handleSlowStream function if we are in the
Backfilling state and backfillTask is not running then we called
scheduleBackfill_UNLOCKED.

However although the backfillTask is not running there could still
be items in the Ready Queue.

As we are still in the Backfilling state, regardless as to whether
the backfillTask is running, we should drop the current cursor
and set the pendingBackfill flag.

Change-Id: I7d8c19041b9cec10640c4ef72c5d62cd73985ea4
Reviewed-on: http://review.couchbase.org/83533
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years ago[BP] MB-25798: Re-register a dropped cursor if we don't backfill 31/83531/2
Daniel Owen [Wed, 8 Mar 2017 12:28:17 +0000 (12:28 +0000)]
[BP] MB-25798: Re-register a dropped cursor if we don't backfill

After dropping a cursor, when scheduleBackfill_UNLOCKED is called but
the backfill task does not need to be scheduled, it means the cursor is
not re-registered in markDiskSnapshot.

Therefore the cursor must therefore be re-registered from within
scheduleBackfill_UNLOCKED.

Change-Id: I4a643aed902316e0753555564b8bfd9b56291efe
Reviewed-on: http://review.couchbase.org/83531
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years ago[BP] MB-25798: Backfill task leave stream state unchanged 24/83524/3
Daniel Owen [Tue, 28 Feb 2017 12:22:13 +0000 (12:22 +0000)]
[BP] MB-25798: Backfill task leave stream state unchanged

The backfill task must not change the state machine of the active
stream that it is associated with.  In particular:
1) In ActiveStream::markDiskSnapshot if a vbucket is not found then
   it should just return, leaving the entity deleting the vbucket to
   set the stream to dead.
2) ActiveStream::completeBackfill should not call
   scheduleBackfill_UNLOCKED as this can move the stream into the
   STREAM_IN_MEMORY state.

State machine changes should only be driven by ActiveStream::next,
which is invoked by DcpProducer::getNextItem.

The call to scheduleBackfill_UNLOCKED that was invoked in
ActiveStream::completeBackfill when the pendingBackfill flag was true
has been moved to the ActiveStream::backfillPhase function and is
invoked once the current backfill has completed and the pendingBackfill
flag is set to true.

Change-Id: Idcbf8164792f4fd09898fe90748424687c60fb6a
Reviewed-on: http://review.couchbase.org/83524
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years ago[BP] MB-25798: Log when DcpProducer::Buffer log is full 21/83521/4
Manu Dhundi [Tue, 19 Sep 2017 20:40:59 +0000 (13:40 -0700)]
[BP] MB-25798: Log when DcpProducer::Buffer log is full

Log when unable to notify a paused connection because the BufferLog is
full.  And log when space becomes available in the BufferLog allowing a
paused connection to be notified.

Also log when ignoring a request to notify that a stream is ready due
to the associated vbucket already being in the ready queue.

Change-Id: I75f2d25af5d0f11175beb7b23300486092aa86fa
Reviewed-on: http://review.couchbase.org/83521
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years ago[BP] MB-25798: Log seqno data for when not scheduling backfill 20/83520/4
Daniel Owen [Thu, 23 Feb 2017 14:18:37 +0000 (14:18 +0000)]
[BP] MB-25798: Log seqno data for when not scheduling backfill

When ActiveStream::scheduleBackfill_UNLOCKED is invoked with reschedule
defined to true it is rare for a backfill not to be scheduled.

In this case it is useful to log seqno data.

Change-Id: I145da4f69db3b81e77a4e85ff65ad070edf21c0e
Reviewed-on: http://review.couchbase.org/83520
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years ago[BP] MB-25798: Improved logging for handle slow stream & scheduling backfill 19/83519/3
Daniel Owen [Tue, 21 Feb 2017 17:57:58 +0000 (17:57 +0000)]
[BP] MB-25798: Improved logging for handle slow stream & scheduling backfill

An example of the cause of the issue reported in MB-22451 is provided
below:

1/ DCP (Producer) eq_dcpq:replication:ns_1@172.23.96.101->ns_1@172.23.96.102:bucket-1 - (vb 399) Creating stream with start seqno 0 and end seqno 18446744073709551615
2/ DCP (Producer) eq_dcpq:replication:ns_1@172.23.96.101->ns_1@172.23.96.102:bucket-1 - (vb 399)  Producer is handling slow stream; state:in-memory lastReadSeqno:548720 lastSentSeqno:538712
3/ DCP (Producer) eq_dcpq:replication:ns_1@172.23.96.101->ns_1@172.23.96.102:bucket-1 - (vb 399) Scheduling backfill from 548721 to 573764, reschedule flag : True
4/ DCP (Producer) eq_dcpq:replication:ns_1@172.23.96.101->ns_1@172.23.96.102:bucket-1 - (vb 399) Sending disk snapshot with start seqno 0 and end seqno 573764
5/ DCP (Producer) eq_dcpq:replication:ns_1@172.23.96.101->ns_1@172.23.96.102:bucket-1 - (vb 399)  Producer is handling slow stream; state:backfilling lastReadSeqno:549860 lastSentSeqno:549140
6/ DCP (Producer) eq_dcpq:replication:ns_1@172.23.96.101->ns_1@172.23.96.102:bucket-1 - (vb 399) Backfill complete, 3989 items read from disk, 21055 from memory, last seqno read: 573764

A backfill is scheduled (step 3) and then before it completes, another
call is made to handle slow stream (step 5). This should result in
setting the pendingBackfill flag to true and then when the backfill
completes (step 6) it should schedule another backfill.
However this second backfill is not scheduled.

This patches adds improved logging which will provide the additional
information needed to debug this and similar issues.

Change-Id: I4a241c8f01c9dc828564649a5bdfd24be853308b
Reviewed-on: http://review.couchbase.org/83519
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years agoMB-25630: Include read-only KVStore stats in 'cbstats kvtimings' 11/82911/2
Dave Rigby [Wed, 30 Aug 2017 11:05:35 +0000 (12:05 +0100)]
MB-25630: Include read-only KVStore stats in 'cbstats kvtimings'

During investigation of slow background fetches on a customer
environment, I found that while we record filesystem timings for both
read-only and read-write KVStores, we only report the timings for
read-write.

To better assist in analysing read timings, we should also include the
read-only instance in the stats (accessed via the kvtimings cbstats
group).

Change-Id: Ie505e194cb1a91c80e8551cb198636ad68f56f1c
Reviewed-on: http://review.couchbase.org/82911
Tested-by: Build Bot <build@couchbase.com>
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
3 years agoMB-25464: Do not exit cbstats if stats_perform fails 27/81627/2 v4.6.3
Dave Rigby [Mon, 31 Jul 2017 17:06:16 +0000 (18:06 +0100)]
MB-25464: Do not exit cbstats if stats_perform fails

Exiting early will prevent cbstats collecting stats from other buckets
when running with `-a`.

Change-Id: I36641b06c5c22ea0add46b3bbe91332ec0fc498a
Reviewed-on: http://review.couchbase.org/81627
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: James Harrison <james.harrison@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years agoMB-25238: Add logging to destruction of store 81/81481/3
Jim Walker [Thu, 27 Jul 2017 15:54:15 +0000 (16:54 +0100)]
MB-25238: Add logging to destruction of store

The MB shows lots of time missing between unregisterTaskable and
back in memcached when EvpDestroy has returned, this covers deletion
of various objects and interestingly the destruction and flush of the
mutation log objects.

Change-Id: I3dd7913a5fb02e777b931a11eac97657584b79ee
Reviewed-on: http://review.couchbase.org/81481
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years ago[BP] MB-25337: Handle the vbucket state change gracefully during rollback 38/81238/2
Jim Walker [Fri, 21 Jul 2017 11:16:42 +0000 (12:16 +0100)]
[BP] MB-25337: Handle the vbucket state change gracefully during rollback

Rollback is done asynchronously in kv-engine. When a scheduled rollback
task is run, the vbucket state might have already changed to
non-replica. Upon such a condition, rollback task must just finish
running (as a null operation) rather than throwing an exception.

Change-Id: I459768be3727ca19e141a917e38892f91b5e43f9
Reviewed-on: http://review.couchbase.org/81238
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years agoMB-24817: During takeover, hold stream lock until vb is set to dead 32/79532/4
Manu Dhundi [Thu, 15 Jun 2017 15:42:50 +0000 (08:42 -0700)]
MB-24817: During takeover, hold stream lock until vb is set to dead

During DCP takeover, when we decide to set the vbucket to dead state
on the producer side, we also transition the stream state from
STREAM_TAKEOVER_WAIT to STREAM_TAKEOVER_SEND state to send out any
remaining items that were received before the vbucket was set to
dead state.

The stream lock must be held until the vbucket is set to the dead
state, so that we do not prematurely finish sending out the last
items when the vbucket is not dead yet (that is if the vbucket
is not dead, it could get some more items).

This commit addresses the issue and also handles the ordering of
locks involved at stream level, ep store level and vbucket level
in the scenario.

Change-Id: I89bb42edec4f3765c8a9c67e6e89e9680eb40875
Reviewed-on: http://review.couchbase.org/79532
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years agoMB-24148: Increase the backfill buf to avert hangs with large items 74/77474/4
Manu Dhundi [Fri, 28 Apr 2017 00:18:09 +0000 (17:18 -0700)]
MB-24148: Increase the backfill buf to avert hangs with large items

Currently we have a bug in backfill buffer accounting wherein
there could be a hang potentially with large items whose size are
almost equal to default max.

This commit addresses the bug by masking it. That is we increase the
default backfill buffer size to a value such that we cannot hit the
bug with default item max size.

We are just masking the bug to reduce the amount of change on this
maintainence branch. On the master we are fully fixing the bug.

Change-Id: I215c496cf92a9e9722291ac9a2ad6d14a0298f5c
Reviewed-on: http://review.couchbase.org/77474
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years agoMB-24066/MB-22178: Set opencheckpointid to 1 after rollback 37/77837/3
Daniel Owen [Mon, 8 May 2017 17:00:09 +0000 (18:00 +0100)]
MB-24066/MB-22178: Set opencheckpointid to 1 after rollback

An opencheckpoint of 0 has the special meaning of being in a backfill
phase.

Therefore after performing a rollback we must ensure the
opencheckpointid is reset to 1.  This is the value used when the
vbucket is originally intialised.

Change-Id: I6b97faa7b502406961a4b48ad7affdbf6bc30512
Reviewed-on: http://review.couchbase.org/77837
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years agoMB-24066: Partial revert "MB-22178: Don't use opencheckpointid to determine if in... 04/77604/6
Daniel Owen [Tue, 2 May 2017 12:46:50 +0000 (13:46 +0100)]
MB-24066: Partial revert "MB-22178: Don't use opencheckpointid to determine if in backfill phase"

Revert functional changes in
Change-Id: Ia977d6bf90e54fd1ceb8db4a9088b19d94d4bc8c,
which although addressed the rollback bug described in MB-22178, caused
the bug described in MB-24066.

The tests added in Ia977d6bf90e54fd1ceb8db4a9088b19d94d4bc8c remain but
the rollback test has been disabled as it attempts a streamRequest when
the vbucket has a opendcheckpointid of 0.

The test will be re-enabled and modified with a follow-up patch that
addresses the bug described in MB-22178.

Change-Id: Ifd11c77a10e4ebe571c50e5d518403b423c3dfa5
Reviewed-on: http://review.couchbase.org/77604
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years agoMB-23591: Re-instate isBucketDeletion check for flusher 86/76186/2 v4.6.2 v4.6.2-MP1 v4.6.2-MP2 v4.6.2-MP3
Jim Walker [Mon, 3 Apr 2017 08:58:38 +0000 (09:58 +0100)]
MB-23591: Re-instate isBucketDeletion check for flusher

The VBDeleteTask maybe scheduled to run, and isBucketDeletion
flag indicates if so, thus the flusher still needs to check
it to ensure no writes occur to a file that is about to be
deleted.

Change-Id: I746eb372f35c9b838dfab2ee7ac2b6c59606e94a
Reviewed-on: http://review.couchbase.org/76186
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: David Haikney <david.haikney@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years agoMB-23503: With an unpersisted snapshot, remove HT items till rollback point 28/75628/3
Manu Dhundi [Thu, 23 Mar 2017 18:52:06 +0000 (11:52 -0700)]
MB-23503: With an unpersisted snapshot, remove HT items till rollback point

When the rollback request intends to have a rollback to a point in
an unpersisted snapshot, we must remove all unpersisted checkpoint
items from both checkpoint and hash table till the last persisted
disk snapshot.

Prior to this commit, we removed the items from the checkpoint
correctly, but only removed hash table items till the requested
rollback point, not all the unpersisted items, when
requested_rollback_seqno > persisted_seqno.

Note: We currently always rollback to a point which is persisted on
disk. Hence we must drop all checkpoint items from checkpoint and
hash table, irrespective of whether they are part of a full snapshot
or partial snapshot.

Change-Id: I1c14d0df6ae5e5459e60ecb0fc4a72ecd14231d3
Reviewed-on: http://review.couchbase.org/75628
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years agoMB-23112 Fix deleteWithMeta and extended meta data input 29/74529/4
Jim Walker [Thu, 2 Mar 2017 16:28:35 +0000 (16:28 +0000)]
MB-23112 Fix deleteWithMeta and extended meta data input

An incorrect offset meant that we tried to parse an incorrect
part of the packet as ExtendedMetaData.

Test code updated to cover this case.

Change-Id: If0610fe73b378e5830b4f4b9978d2b7507b516b4
Reviewed-on: http://review.couchbase.org/74529
Reviewed-by: David Haikney <david.haikney@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Well-Formed: Build Bot <build@couchbase.com>

3 years agoMB-22180: Ensure all pendingBGFetches are terminated on VBbucket delete 11/73811/20
Daniel Owen [Mon, 19 Dec 2016 04:17:33 +0000 (04:17 +0000)]
MB-22180: Ensure all pendingBGFetches are terminated on VBbucket delete

On a VBucket delete we need to ensure that all pendingBGFetches are
terminated and any connections waiting for a BGFetch to complete are
notified with ENGINE_NOT_MY_VBUCKET.

Although we previously deleted the pendingBGFetches in the vbucket
destructor we did not send any notifications to waiting connections.

This patch moves the deletion of pendingBGFetches into the
notifyAllPendingConnsFailed function and in addition notifies all the
connections awaiting a BGFetch with an ENGINE_NOT_MY_VBUCKET.

Change-Id: I13a99fe01153a4ba8786aaf608b25ed31ace5a0c
Reviewed-on: http://review.couchbase.org/73811
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
3 years agoMB-22178: Don't use opencheckpointid to determine if in backfill phase 13/73813/3
Daniel Owen [Thu, 8 Dec 2016 15:59:44 +0000 (15:59 +0000)]
MB-22178: Don't use opencheckpointid to determine if in backfill phase

The opencheckpointid of a bucket can be zero after a rollback.
If an opencheckpointid was zero it was assumed that the vbucket was in
backfilling state.  This caused the producer stream request to be stuck
waiting for backfilling to complete.

Ths patch uses the vb->isBackfillPhase() to determine if the vbucket is
in a backfill state as opposed to using an opencheckpointid of zero.

Change-Id: Ia977d6bf90e54fd1ceb8db4a9088b19d94d4bc8c
Reviewed-on: http://review.couchbase.org/70810
Reviewed-by: Manu Dhundi <manu@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Reviewed-on: http://review.couchbase.org/73813
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
3 years agoMB-22256: Fix intermittent failures in test_duplicate_items_disk 58/73758/3
Daniel Owen [Thu, 16 Feb 2017 17:39:47 +0000 (17:39 +0000)]
MB-22256: Fix intermittent failures in test_duplicate_items_disk

The ep_testsuite: test_duplicate_items_disk test occassionally fails
because waiting for the ep_vbucket_del stat to update (from 0 to 1)
times-out.

The reason the stat occasionally does not increase is because the
the vbucket that is undergoing deletion is set back to active before
the deletion is complete.

A side-effect of setting a vbucket to active, is that if the vbucket
does not exist then it is recreated.

This patch moves the setting of the vbucket back to active to after we
have confirmed the deletion has completed (by ensuring the
ep_vbucket_del stat increases).  Thereby removing the race condition.

Change-Id: Ied680c64536862829974fd061a25d317f766bb46
Reviewed-on: http://review.couchbase.org/73758
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
3 years agoMB-22451: Ensure isBackfillTaskRunning is correctly set 10/73310/10
Daniel Owen [Fri, 3 Feb 2017 14:33:43 +0000 (14:33 +0000)]
MB-22451: Ensure isBackfillTaskRunning is correctly set

In ActiveStream::completeBackfill if in STREAM_BACKFILLING state and
pendingBackfill is true then we will schedule another backfill.  This
will cause isBackfillTaskRunning to be set to true.  The flag should
remain true on exit of the completeBackfill function.

Change-Id: If8219a7f87b65af46d37a800eebf2257917cc555
Reviewed-on: http://review.couchbase.org/73310
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoMB-22235: Drift thresholds don't get updated from set_param 00/72100/3
Jim Walker [Mon, 9 Jan 2017 14:13:39 +0000 (14:13 +0000)]
MB-22235: Drift thresholds don't get updated from set_param

Need to plug the change value listener into the bucket config
object.

Test updated to look at the vbucket's threshold values

Change-Id: Id515d71fe551474e93ff5113148ff8411ed739fb
Reviewed-on: http://review.couchbase.org/72100
Well-Formed: buildbot <build@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
4 years agoMB-21925: Fix queue_fill stat when persitenceCursor on queue_op::empty 30/70730/3 v4.6.0 v4.6.0-MP1
Dave Rigby [Wed, 7 Dec 2016 17:44:03 +0000 (17:44 +0000)]
MB-21925: Fix queue_fill stat when persitenceCursor on queue_op::empty

There is a bug related to how we account for items when we enqueue
mutations to be persisted (Checkpoint::queueDirty). The bug occurs
when we enqueue an item which already exists (i.e. it is to be
de-duplicated), and the persistence cursor has not permitted any items
for that VBucket - i.e. at the very beginning of the lifetime of the
VBucket.

With the advent of the MB-20852 (the set_vbucket_state changes), the
first item in a Bucket's checkpoint is now a
queue_op::set_vbucket_state - previously it would have been a "normal"
item. When items are added to a checkpoint, we just update the queuing
stats - essentially there are two possibilities:

1. The item is a new key (NEW_KEY), or it's a key which isn't new but
   has already been persisted (PERSIST_AGAIN) - we need to persist it -
   increment queue_fill

2. The item can be de-duplcated (EXISTING_ITEM) - don't increment
   queue_fill.

Essentially, there's a bug in this logic - we incorrectly mark the
incoming item as PERSIST_AGAIN, instead of EXISTING_ITEM. As a result
the queue_fill value is incorrectly inflated.

This is due to using a unsigned value for the mutation_id, which we
decrement by one if it's a meta_item. This should go to -1 (less than
any possible valid item id), but instead it goes to 2^63-1, *greater*
than any possible valid item (!)

Note: this is purely a stats issue, the actual item /is/ queued and
persisted correctly.

Change-Id: I8d40c417dc165e192b4f90e8cc16c6e87557d451
Reviewed-on: http://review.couchbase.org/70730
Tested-by: buildbot <build@couchbase.com>
Well-Formed: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
4 years agoMB-21867: test_item_pager: Retry on ENOMEM when fetching items 15/70515/2
Dave Rigby [Thu, 1 Dec 2016 16:24:51 +0000 (16:24 +0000)]
MB-21867: test_item_pager: Retry on ENOMEM when fetching items

Change-Id: I9031ca1ff51aed609144ccf141897354138c2f88
Reviewed-on: http://review.couchbase.org/70515
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
4 years agoMB-21790: CheckpointTest.SeqnoAndHLCOrdering needs 1 checkpoint 50/70350/2
Jim Walker [Thu, 24 Nov 2016 16:45:38 +0000 (16:45 +0000)]
MB-21790: CheckpointTest.SeqnoAndHLCOrdering needs 1 checkpoint

Configure the test so that a single checkpoint is created, otherwise
the time-based closing code may split the data over more than 1
checkpoint causing the final data checks to fail.

Change-Id: I449a80d775b8d21afd9b46387a432dce7dcec2d1
Reviewed-on: http://review.couchbase.org/70350
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21769: Fix vbstate snap start/end off by one 40/70340/4
Jim Walker [Thu, 24 Nov 2016 10:41:25 +0000 (10:41 +0000)]
MB-21769: Fix vbstate snap start/end off by one

Recent changes to vbstate writing introduced an edge case regression.
If we force shutdown and had empty active vbuckets, after warmup
we will expose an incorrect failover table as we have an entry
for seq 1, yet the VB has high-seq of 0.

Change-Id: Iee67f71ce46c8eaf4f2cd822103354dcdecc04d8
Reviewed-on: http://review.couchbase.org/70340
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB_21787: Remove log message from couch-kvstore for non-existent key 45/70345/2
Dave Rigby [Thu, 24 Nov 2016 14:54:55 +0000 (14:54 +0000)]
MB_21787: Remove log message from couch-kvstore for non-existent key

If an arithmetic operation is performed on a key which isn't resident,
on a full-eviction bucket then the following message is often printed
in the logs:

    WARNING (default) Failed to fetch data from database, vBucket=0
    key=Key_19 error=document not found [none]

This is unnecessarily verbose - this is not actually a warning in this
case and can quickly fill up the logs.

(Note: the reason this is shown is that prior to full-eviction, all
metadata would be resident and hence it would be unexpected to attempt
to read from disk a key which doesn't exist). However this is no
longer the case).

Change-Id: I6ac4f2b1f8d43700ff6869da8a6670e21454b3a0
Reviewed-on: http://review.couchbase.org/70345
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoMB-21777: max_cas_str not formatting on Linux 98/70298/2
Jim Walker [Wed, 23 Nov 2016 15:28:05 +0000 (15:28 +0000)]
MB-21777: max_cas_str not formatting on Linux

Seems an issue with snprintf where we use timeString as input and
output yields only the nanosecond fractions being printed.

Fix also reduces buffers down, they're still bigger than we need, but
100 is way too much.

Change-Id: Ief6755d15d4d77a10a6c14fb76321893a7dd0d9c
Reviewed-on: http://review.couchbase.org/70298
Reviewed-by: David Haikney <david.haikney@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
4 years agoSplit test_curr_items test into 4 smaller tests 52/70152/3
Dave Rigby [Mon, 21 Nov 2016 13:20:49 +0000 (13:20 +0000)]
Split test_curr_items test into 4 smaller tests

test_curr_items performs a number of different checks on the
curr_items stat. To simplify diagnosing issues with it (there's been a
number of intermittent failures), break it into four sub-tests.

Change-Id: I2ecd4bcacf8ca4cb7add65ec72a71e2a17545a24
Reviewed-on: http://review.couchbase.org/70152
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoRemove duplicate full_eviction tests from ep_testsuite 51/70151/3
Dave Rigby [Mon, 21 Nov 2016 13:12:29 +0000 (13:12 +0000)]
Remove duplicate full_eviction tests from ep_testsuite

Now we run all ep testsuites in with full and value eviction modes,
there is no need to run some specific tests in full_eviction
mode. Remove them.

Change-Id: I58256f28173b4113a9818fb7b514aceda002d7ec
Reviewed-on: http://review.couchbase.org/70151
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21765: Missing lock from doWorkerStat 62/70262/3
Daniel Owen [Tue, 22 Nov 2016 16:35:47 +0000 (16:35 +0000)]
MB-21765: Missing lock from doWorkerStat

tMutex must be acquired before accessing the threadQ data structure.

07:01:46 WARNING: ThreadSanitizer: data race (pid=32512)
07:01:46 Write of size 8 at 0x7d1c00016eb8 by thread T9 (mutexes: write M27337):
07:01:46 #0 operator delete(void*) <null> (engine_testapp+0x000000464cbb)
07:01:46 #1 <null> <null> (libstdc++.so.6+0x0000000c1ac7)
07:01:46 #2 ExecutorThread::run() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/ep-engine/src/executorthread.cc:127 (ep.so+0x0000000f7ff5)
07:01:46 #3 launch_executor_thread(void*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/ep-engine/src/executorthread.cc:32 (ep.so+0x0000000f78a5)
07:01:46 #4 CouchbaseThread::run() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/platform/src/cb_pthreads.cc:58 (libplatform_so.so.0.1.0+0x000000007b91)
07:01:46 #5 platform_thread_wrap(void*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/platform/src/cb_pthreads.cc:71 (libplatform_so.so.0.1.0+0x000000005e92)
07:01:46
07:01:46 Previous read of size 8 at 0x7d1c00016eb8 by main thread (mutexes: write M20977, write M26658):
07:01:46 #0 strlen <null> (engine_testapp+0x0000004652af)
07:01:46 #1 showJobLog(char const*, char const*, std::vector<TaskLogEntry, std::allocator<TaskLogEntry> > const&, void const*, void (char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/ep-engine/src/statwriter.h:39 (ep.so+0x0000000f346b)
07:01:46 #2 ExecutorPool::doWorkerStat(EventuallyPersistentEngine*, void const*, void (char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/ep-engine/src/executorpool.cc:801 (ep.so+0x0000000f2ce2)
07:01:46 #3 EventuallyPersistentEngine::getStats(void const*, char const*, int, void (char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-master/ep-engine/src/ep_engine.cc:4383 (ep.so+0x0000000c30bf)

Change-Id: Iae7aa0fe9da15805671762516221f25da69fe5d8
Reviewed-on: http://review.couchbase.org/70262
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoFix error in Blob::operator== 68/70168/2
Dave Rigby [Mon, 21 Nov 2016 17:22:17 +0000 (17:22 +0000)]
Fix error in Blob::operator==

Fix typo in Blob::operaror== - incorrect comparision of
extMetaLen. Introduced by http://review.couchbase.org/69477

Bug identified by Coverity:

    CID 153111:  Incorrect expression  (CONSTANT_EXPRESSION_RESULT)
    "lhs.extMetaLen == lhs.extMetaLen" is always true regardless of
    the values of its operands because those operands are identical. This
    occurs as the logical second operand of "&&".

Change-Id: I87529e6a3801358bafe265869125c2eff2d40587
Reviewed-on: http://review.couchbase.org/70168
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agoMB-21738: Fix potential crash due to race deleting VBucket 63/70163/3
Dave Rigby [Mon, 21 Nov 2016 15:09:29 +0000 (15:09 +0000)]
MB-21738: Fix potential crash due to race deleting VBucket

There is a potential race condition in
VBucketMap::setPersistenceCheckpointId during VBucket deletion which
can result in dereferencing a deleted pointer, triggering a segfault.

The issue is that setPersistenceCheckpointId can dereference a RCPtr
which has just become null. The issue is on line 177 - we dutifully
check if is valid, but then re-fetch the VBucket - at which point it
may have been set to null by another thread (such as when a VBucket is
deleted).

Fix is to just use the local `vb` to dereference.

Change-Id: I683cb0d0cfe37e710e98ba6bbf1ddd4cf3682a35
Reviewed-on: http://review.couchbase.org/70163
Reviewed-by: David Haikney <david.haikney@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21599: Prevent LWW XDCR from non LWW cluster 16/70016/12
Jim Walker [Thu, 17 Nov 2016 12:35:43 +0000 (12:35 +0000)]
MB-21599: Prevent LWW XDCR from non LWW cluster

Add support for FORCE_ACCEPT_WITH_META_OPS
 - This flag must be set if the bucket is lww
 - Error if the bucket is !lww

Add support for REGENERATE_CAS
 - This flag requires SKIP_CONFLICT_RESOLUTION
 - When the item is stored, its CAS is re-created.

Tidy up test code relating to *_with_meta and add more extensive
testing of options and nmeta.

Change-Id: Ifb149927d3f63357d30392352c9c81533cbf2ff1
Reviewed-on: http://review.couchbase.org/70016
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
4 years agoMB-21539: Address raciness / intermittent timeout in test_mb16357 85/70085/7
Dave Rigby [Fri, 18 Nov 2016 15:29:52 +0000 (15:29 +0000)]
MB-21539: Address raciness / intermittent timeout in test_mb16357

This is a multithreaded test which attempts to run the compactor
concurrently with a DCP stream when a vBucket flips to
replica. However, there is no interlocking of the threads' startup,
which can result in the compaction running and completing before the
DCP thread reaches line 810 (where it busy-waits for
ep_pending_compactions != 0). As a result the test can get stuck at
that line and timeout.

Address this by introducing synchronisation between the two threads -
ensure that the compactor thread is ready and about to compact before
advancing the DCP thread.

Change-Id: Icc5abb3080a981823bb2161d083f0d866364b76e
Reviewed-on: http://review.couchbase.org/70085
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoMB-21716: Consistent drift_threshold parameters 83/70083/8
Jim Walker [Fri, 18 Nov 2016 12:53:51 +0000 (12:53 +0000)]
MB-21716: Consistent drift_threshold parameters

Change hlc_ahead to hlc_drift_ahead

Same for behind parameters.

Change-Id: I9331772c736767fecc82350087f581cdf13801a9
Reviewed-on: http://review.couchbase.org/70083
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21685: Use EWOULDBLOCK variant of delVBucket 82/70082/7
Jim Walker [Tue, 15 Nov 2016 16:04:23 +0000 (16:04 +0000)]
MB-21685: Use EWOULDBLOCK variant of delVBucket

The test can occasionally fail because a delete overlaps
the test code which has set the VB active and written data.
The data in the overlap gets deleted from disk.

MB-21687 covers the overlap issue for delVBucket without the async=0
parameter.

Change-Id: I74666d087aab65456110d7815f701e4193fc71b9
Reviewed-on: http://review.couchbase.org/70082
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21724: Reduce iterations in CheckpointTest basic_chk_test 86/70086/6
Jim Walker [Fri, 18 Nov 2016 15:37:12 +0000 (15:37 +0000)]
MB-21724: Reduce iterations in CheckpointTest basic_chk_test

Reduce number of items and reduce threads and items further when
running under valgrind.

Also removed a sleep(1) and fixed the thread start/wait code
that relied on the sleep(1).

Change-Id: Ie6d71bf0972e30225343c12c51e36d5adadec794
Reviewed-on: http://review.couchbase.org/70086
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
4 years agoMB-21725: test_exp_persisted_set_del check for greater or equal 1 91/70091/2
Jim Walker [Fri, 18 Nov 2016 16:43:17 +0000 (16:43 +0000)]
MB-21725: test_exp_persisted_set_del check for greater or equal 1

The test stores key twice and waits for ep_total_persisted to equal 1.
The test fails because the wait_for stat function timesout, the
total persisted is 2.

If the flusher ran quickly we may actually have a value of 2.

Change-Id: I18d8e50e5a5bcc4839830adb30e104bb6796daa0
Reviewed-on: http://review.couchbase.org/70091
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21540: ep-engine: Remove abort() 76/69376/11
Dave Rigby [Tue, 1 Nov 2016 16:39:05 +0000 (16:39 +0000)]
MB-21540: ep-engine: Remove abort()

Final set of changes to remove abort() from ep_engine production code;
replacing with exceptions where a dynamic error is still required.

All non-test ep-engine code is now abort-free :)

Change-Id: I9702b6bbaf28267b696498067318e78af0988002
Reviewed-on: http://review.couchbase.org/69376
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21568: Don't double-free newdb if rewind_db_header fails 80/70080/3
Dave Rigby [Fri, 18 Nov 2016 12:31:19 +0000 (12:31 +0000)]
MB-21568: Don't double-free newdb if rewind_db_header fails

If couchstore_rewind_db_header() fails, then it will free the DB
before returning, so we need to ensure that the DbHolder doesn't cause
a double-free by also trying to free the Db.

Found during merge to master where we have improved couchstore testing
(CouchKVStoreErrorInjectionTest)

Change-Id: I49ba9e7e91eb73aaae90ef8aa8b41f56bbe056c9
Reviewed-on: http://review.couchbase.org/70080
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-19929: Fix sporadic failure in 'dcp cursor dropping' test 84/70084/2
Manu Dhundi [Fri, 17 Jun 2016 21:29:26 +0000 (14:29 -0700)]
MB-19929: Fix sporadic failure in 'dcp cursor dropping' test

In cursor dropping test the max_size and chk_max_items should be such
that 2 checkpoints are created. The test simulates a case where of
the 2 checkpoints, the DCP cursor from one of them is dropped.

Change-Id: Ieb5c6dfba43bea2f8256fa01104f9ddf83a9781f
Reviewed-on: http://review.couchbase.org/70084
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoMB-21650: Prevent false sharing of frequently modified memory stats 51/69951/8
Dave Rigby [Wed, 16 Nov 2016 17:59:55 +0000 (17:59 +0000)]
MB-21650: Prevent false sharing of frequently modified memory stats

We record the memory usage, memory overhead and number of Items per
bucket, in the EPStats object. These stats are very frequenly updated
(on every memory allocation/deallocation, and every Item
creation/destruction), and they are updated from all threads. This can
cause false sharing between these values if they co-exist in the same
cache line.

A recent change (66882e8 - MB-20852 [17/N]: Serialize VB state
changes) added a new element to EPStats which resulted in the layout
of that class changing such that memory stat variables (memOverhead,
numItem, totalMemory) ended up on the same cache line. This resulted
in a signficant regression (1.9M op/s -> 1.4M ops) in the performance
of the 'kv_max_ops_reads_512_avg_ops_hera_kv' test, due to false
sharing between these variables.

To address this, ensure that each variable is placed on its own cache
line to prevent false sharing, by using the CachelinePadded template
class.

Change-Id: I8316637a7a0c6fd05fd6f6ba24a1df44c43390c5
Reviewed-on: http://review.couchbase.org/69951
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21650: Don't copy key when servicing memcached get() request 23/70023/5
Dave Rigby [Wed, 16 Nov 2016 17:37:13 +0000 (09:37 -0800)]
MB-21650: Don't copy key when servicing memcached get() request

In the get request path (EvpGet() and onwards) we take a copy of the
key from the request packet into a std::string. This is then passed
around ep-engine (read-only) to retrieve the document to return to the
user.

This incurs the cost of creating a std::string object per get request,
along with the related memory tracking overheads which are not
insigificant (see ObjectRegistry).

To reduce this cost, use a const_sized_buffer (essentially a pair of
{const char*, size_t}) for the key, relying on the fact that the
client's request is always available in the request packet owned by
memcached.

On the 2-node 'hera' cluster this improves the op/s of the following
pillowfight benchmark, from 1.8M op/s total to 2.3M op/s:

    cbc-pillowfight --spec couchbase://172.23.96.117:12000/default \
        --batch-size 1000 --num-items 1000000 --num-threads 50 \
         --min-size 512 --max-size 512 --set-pct 20

(client running on 3rd hera-client node).

Change-Id: I0371fb5ef9bdcdc6f92bb941926e8af80cf5e24f
Reviewed-on: http://review.couchbase.org/70023
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20079: Ensure updateWaketimeIfLessThan performs a compare_exchange 27/70027/4
Daniel Owen [Thu, 17 Nov 2016 17:00:25 +0000 (17:00 +0000)]
MB-20079: Ensure updateWaketimeIfLessThan performs a compare_exchange

The patch MB-20079: Use std::chrono::steady_clock (ProcessClock) with
changeID: I2fc9688abb782fe2c9e80efb6da840be3643d4a5 introduced a bug
where the waketime could be incorrectly updated, due to not performing
a compare_exchange_strong.

This patch reverts to using a compare_exchange_strong, ensuring that
waketime is only updated if it is less than the input time_point.

Change-Id: Ib68689f9a76c6a0455422c293ad62e9c4d04f3d7
Reviewed-on: http://review.couchbase.org/70027
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21568: Lock inversion issue in rollback 29/70029/3
Jim Walker [Thu, 17 Nov 2016 15:45:42 +0000 (15:45 +0000)]
MB-21568: Lock inversion issue in rollback

vbsetMutex must be obtained before vb::stateMutex.

the rollback path needs to keep both held until
complete so some refactoring to expose
_UNLOCKED variants of setVBucketState and resetVbucket
so there's no subsequent inversion risk

Change-Id: I16d869277ad5609b6b45042ea32b3f1037faeb72
Reviewed-on: http://review.couchbase.org/70029
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21568: Use signed int64 when comparing against item bySeqno 24/70024/2
Jim Walker [Thu, 17 Nov 2016 16:49:19 +0000 (16:49 +0000)]
MB-21568: Use signed int64 when comparing against item bySeqno

addresses compiler warning

Change-Id: I63ae0f15baf6512679ca32491b707500309b09e9
Reviewed-on: http://review.couchbase.org/70024
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Dave Rigby <daver@couchbase.com>
4 years agoMB-21568: Reconcile hashtable with disk following rollback 25/69725/16
Dave Rigby [Mon, 7 Nov 2016 06:36:42 +0000 (22:36 -0800)]
MB-21568: Reconcile hashtable with disk following rollback

After rolling back the disk store to the requested seqno a scan of the
vbucket's checkpoint must occur. Any item in the checkpoint with
a seqno > than the rollback must be dropped or rolled back.

+ A missing close is addressed by using a wrapper class that will
  RAII close each file opened in the rollback code.

Change-Id: Iabe43f59ed40931c1c97b65147b7a414d4ff7cc5
Reviewed-on: http://review.couchbase.org/69725
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21692: Improved stability of "temp item deletion" test 41/69941/3
Jim Walker [Wed, 16 Nov 2016 10:53:47 +0000 (10:53 +0000)]
MB-21692: Improved stability of "temp item deletion" test

Ensure the expiry pager runs at the end of the test (before final
assertions) and after the meta bgFetches are complete.

Change-Id: I9182ae5a1ea84da6165ceca7cf04ab3cc1b2f1cd
Reviewed-on: http://review.couchbase.org/69941
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20079: Use std::chrono::steady_clock (ProcessClock) 99/69899/2
Daniel Owen [Tue, 15 Nov 2016 10:33:24 +0000 (10:33 +0000)]
MB-20079: Use std::chrono::steady_clock (ProcessClock)

Change task scheduling to use ProcessClock which is not
affected by changes to wall clock time.

Change-Id: I2fc9688abb782fe2c9e80efb6da840be3643d4a5
Reviewed-on: http://review.couchbase.org/69899
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoMB-21540: tapconnection: Replace abort() with exceptions 75/69375/3
Dave Rigby [Tue, 1 Nov 2016 16:35:59 +0000 (16:35 +0000)]
MB-21540: tapconnection: Replace abort() with exceptions

Change-Id: I100af9bca783d3e3f282d315ffcb99915170eff5
Reviewed-on: http://review.couchbase.org/69375
Reviewed-by: David Haikney <david.haikney@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoItem: Add stream and equality operators 77/69477/4
Dave Rigby [Fri, 4 Nov 2016 09:23:06 +0000 (09:23 +0000)]
Item: Add stream and equality operators

Add operator<< and == to Item and the classes Item is built from
(Blob, ItemMetaData).

Change-Id: I40d5edafae777a65a2a80c7286bb77c91fc486cc
Reviewed-on: http://review.couchbase.org/69477
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
4 years agoMB-21587: Rollback: Don't use CAS when rolling back values in HashTable 78/69478/6
Dave Rigby [Fri, 4 Nov 2016 14:00:10 +0000 (14:00 +0000)]
MB-21587: Rollback: Don't use CAS when rolling back values in HashTable

When ep-engine performs a rollback, if a key needs to be reverted to a
previous value (it has been modified since the rollbackSeqno), the
HashTable update fails due to being called incorrectly - it uses a set
with CAS (where the CAS used is the one for the previous item),
whereas the element in the HashTable will have a newer CAS (by
defintion as we are trying to roll it back).

The effect of this is that after rollback, old values for keys still
exist in memory (disk is correct) on the replica vBucket(s). In the
event of a subsequent failover (and promotion of the replica ->
active), incorrect values will be returned for such keys.

Fix by changing the HashTable::set to a non-CAS one.

As part of testing, made use of recently-added equality and output
operators for Item and related classes:

    [ RUN      ] RollbackTest.MB21587_RollbackAfterMutation
    source/ep-engine/tests/module_tests/evp_store_rollback_test.cc:70: Failure
    Value of: *result.getValue()
      Actual: Item[0x10441d860] with key:a
            value:Blob[0x104543180] with size:5 extMetaLen:1 age:0 data: <01 01 n e w>
            metadata:ItemMetaData[0x10441d868] with cas:5407a59650006 revSeqno:2 flags:0 exptime:0
            bySeqno:7 queuedTime:0 vbucketId:0 op:set nru:2
    Expected: item_v1
    Which is: Item[0x7fff5f181ce0] with key:a
            value:Blob[0x104543160] with size:5 extMetaLen:1 age:0 data: <01 01 o l d>
            metadata:ItemMetaData[0x7fff5f181ce8] with cas:5407a59650005 revSeqno:1 flags:0 exptime:0
            bySeqno:6 queuedTime:0 vbucketId:0 op:set nru:2
    Fetched item after rollback should match item_v1
    [  FAILED  ] RollbackTest.MB21587_RollbackAfterMutation (22 ms)

Change-Id: I32577afd0f6f9c79122575f84a76acd00fb6f89a
Reviewed-on: http://review.couchbase.org/69478
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>
4 years agoMB-21617: Change CAS resolution to nanoseconds 08/69708/7
Jim Walker [Tue, 8 Nov 2016 11:15:58 +0000 (11:15 +0000)]
MB-21617: Change CAS resolution to nanoseconds

This gives better compatibility with older datasets and a less
confusing experience if an old cluster did set_with_cas against
a new one.

The CAS is generated as a nanosecond value.

Drift is tracked though in microseconds to give a longer window before
we may overflow the counter. The config thresholds are also all ┬Ás
but converted to ns so we can correctly track drift.

A new max_cas_str is also added to assist the supportability of
LWW, so at a glance you can see what the max_cas means as a
date/time string.

Change-Id: I40fb89add968043aca01b1de103f62319d814a5c
Reviewed-on: http://review.couchbase.org/69708
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21540: ep_time.cc: Replace abort() with std::logic_error 74/69374/3
Dave Rigby [Tue, 1 Nov 2016 16:04:59 +0000 (16:04 +0000)]
MB-21540: ep_time.cc: Replace abort() with std::logic_error

Change-Id: I26ab8f0f92e879ba6fe49e974d79fa020da20a69
Reviewed-on: http://review.couchbase.org/69374
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21540: Convert ep_time.c -> ep_time.cc 73/69373/4
Dave Rigby [Tue, 1 Nov 2016 16:03:35 +0000 (16:03 +0000)]
MB-21540: Convert ep_time.c -> ep_time.cc

In preparation for replacing use of abort() with exceptions.

Change-Id: I8b95d9b5236c2f7735977cbebd1f182765adb2d4
Reviewed-on: http://review.couchbase.org/69373
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21540: DCP: Replace abort() with exceptions 72/69372/5
Dave Rigby [Tue, 1 Nov 2016 15:54:22 +0000 (15:54 +0000)]
MB-21540: DCP: Replace abort() with exceptions

Remove use of abort() when checking for "impossible" logical
situations in DCP. Where possible remove the abort entirely
(e.g. explicilty handle all cases in swtich statements), otherwise
replace with std::logic_error.

Change-Id: I9a45e283562b5e7361f8e9edd4316d9d253b4e05
Reviewed-on: http://review.couchbase.org/69372
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agoMB-21540: Refactor VBucketVisitor to remove need for abort 71/69371/5
Dave Rigby [Tue, 1 Nov 2016 15:42:12 +0000 (15:42 +0000)]
MB-21540: Refactor VBucketVisitor to remove need for abort

VBucketVisitor is used to visit all VBuckets in a Bucket, and is
currently implemented as a derived class of HashTableVisitor - as the
intent is that concrete classes will iterate at two levels - first
over each VBucket, and then for all StoredValues within the HashTable
of that VBucket.

However, not all derived classes actually visit the individual
StoredValues - many just operate at the VBucket level. As such, these
classes do not expect the visit(StoredValue*) method to be called, and
have abort()s on these code paths.

This is undesirable - it should be possible to statically (i.e. at
compile-time) ensure that such functions are never called.

To address this (and remove the abort() calls), modify VBucketVisitor
so it doesn't inherit from HashTableVisitor - essentially it just
deals with visiting at the VBucket level, which *is* true for all
derived classes. For those derived classes which /do/ wish to perform
recursive visiting of StoredValue, the derived class itself can
inherit from HashTableVisitor.

Additionally, as VBucketVisitor only deals with VBucket visiting,
simpify the visiting logic to a "classic" Visitor pattern, making
visitBucket return void (instead of bool), and let the visitBucket()
method itself handle what do with the VBucket is it passed. This means
that the caller (e.g. EventuallyPersistentStore::visit()) no longer
chooses if the HashTable::visit method is called - the derived class
can call that itself if it chooses.

Change-Id: I1dc8724db9e89daff261bccfe22dc6e2db885b22
Reviewed-on: http://review.couchbase.org/69371
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
4 years agoMB-20725: Reset drift stats 22/69422/3
Jim Walker [Wed, 2 Nov 2016 16:00:56 +0000 (16:00 +0000)]
MB-20725: Reset drift stats

When resetting stats, include the drift counters.

Change-Id: I5c0b78a9d885ed6970f219c2089bf34df3da2425
Reviewed-on: http://review.couchbase.org/69422
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20760: Fix data race on VBucket::chkFlushTimeout 34/69234/6 v4.6.0-DP
Dave Rigby [Wed, 26 Oct 2016 10:00:21 +0000 (11:00 +0100)]
MB-20760: Fix data race on VBucket::chkFlushTimeout

As identified by ThreadSantizer:

WARNING: ThreadSanitizer: data race (pid=23560)
  Write of size 8 at 0x7ff76a2e7e50 by thread T52 (mutexes: write M64345, write M85935):
    #0 VBucket::adjustCheckpointFlushTimeout(unsigned long) ep-engine/src/vbucket.cc:393 (ep.so+0x00000016c806)
    #1 VBucket::notifyOnPersistence(EventuallyPersistentEngine&, unsigned long, bool) ep-engine/src/vbucket.cc:332 (ep.so+0x00000016c806)
    #2 EventuallyPersistentStore::flushVBucket(unsigned short) ep-engine/src/ep.cc:3475 (ep.so+0x0000000be736)
    #3 Flusher::flushVB() ep-engine/src/flusher.cc:284 (ep.so+0x0000001100d1)
    #4 Flusher::step(GlobalTask*) ep-engine/src/flusher.cc:183 (ep.so+0x0000001110b7)
    #5 FlusherTask::run() ep-engine/src/tasks.cc:138 (ep.so+0x000000156642)
    #6 ExecutorThread::run() ep-engine/src/executorthread.cc:115 (ep.so+0x000000108d96)
    #7 launch_executor_thread ep-engine/src/executorthread.cc:33 (ep.so+0x000000109675)
    #8 CouchbaseThread::run() platform/src/cb_pthreads.cc:54 (libplatform.so.0.1.0+0x0000000057a5)
    #9 platform_thread_wrap platform/src/cb_pthreads.cc:66 (libplatform.so.0.1.0+0x0000000057a5)

  Previous read of size 8 at 0x7ff76a2e7e50 by thread T9 (mutexes: write M295):
    #0 VBucket::getCheckpointFlushTimeout() ep-engine/src/vbucket.cc:406 (ep.so+0x0000001683e6)
    #1 EventuallyPersistentEngine::doEngineStats(void const*, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) ep-engine/src/ep_engine.cc:3628 (ep.so+0x0000000e9678)
    #2 EventuallyPersistentEngine::getStats(void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) ep-engine/src/ep_engine.cc:4680 (ep.so+0x0000000ea930)
    #3 EvpGetStats ep-engine/src/ep_engine.cc:235 (ep.so+0x0000000eb9fe)
    #4 stat_executor memcached/daemon/mcbp_executors.cc:3367 (memcached+0x00000046c94b)
    #5 process_bin_packet memcached/daemon/mcbp_executors.cc:4650 (memcached+0x00000046481d)
    #6 mcbp_complete_nread(McbpConnection*) memcached/daemon/mcbp_executors.cc:4759 (memcached+0x00000046481d)
    #7 conn_nread(McbpConnection*) memcached/daemon/statemachine_mcbp.cc:314 (memcached+0x000000472678)
    #8 McbpStateMachine::execute(McbpConnection&) memcached/daemon/statemachine_mcbp.h:43 (memcached+0x000000447054)
    #9 McbpConnection::runStateMachinery() memcached/daemon/connection_mcbp.cc:1003 (memcached+0x000000447054)
    #10 McbpConnection::runEventLoop(short) memcached/daemon/connection_mcbp.cc:1274 (memcached+0x0000004470dd)
    #11 run_event_loop memcached/daemon/connections.cc:147 (memcached+0x00000044b9e9)
    #12 event_handler(int, short, void*) memcached/daemon/memcached.cc:851 (memcached+0x00000041466c)
    #13 event_persist_closure /home/couchbase/serverjenkins/workspace/cbdeps-platform-build/deps/packages/build/libevent/libevent-prefix/src/libevent/event.c:1319 (libevent_core-2.0.so.5+0x00000000b6b7)
    #14 event_process_active_single_queue /home/couchbase/serverjenkins/workspace/cbdeps-platform-build/deps/packages/build/libevent/libevent-prefix/src/libevent/event.c:1363 (libevent_core-2.0.so.5+0x00000000b6b7)
    #15 event_process_active /home/couchbase/serverjenkins/workspace/cbdeps-platform-build/deps/packages/build/libevent/libevent-prefix/src/libevent/event.c:1438 (libevent_core-2.0.so.5+0x00000000b6b7)
    #16 event_base_loop /home/couchbase/serverjenkins/workspace/cbdeps-platform-build/deps/packages/build/libevent/libevent-prefix/src/libevent/event.c:1639 (libevent_core-2.0.so.5+0x00000000b6b7)
    #17 CouchbaseThread::run() platform/src/cb_pthreads.cc:54 (libplatform.so.0.1.0+0x0000000057a5)
    #18 platform_thread_wrap platform/src/cb_pthreads.cc:66 (libplatform.so.0.1.0+0x0000000057a5)

  Location is global 'VBucket::chkFlushTimeout' of size 8 at 0x7ff76a2e7e50 (ep.so+0x000000431e50)

Change-Id: I2d6f928b8a5552cf08a91c6134ad5134810966c2
Reviewed-on: http://review.couchbase.org/69234
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21539: Use correct expiry value for test 'MB-16357' 65/69365/3
Dave Rigby [Tue, 1 Nov 2016 14:22:54 +0000 (14:22 +0000)]
MB-21539: Use correct expiry value for test 'MB-16357'

During the refactor done in b4c858e, the expiration value of 1s was
lost for this test. Restore it.

Change-Id: I0b1bf35548cffcd6867a29dcbb9a843b0f06b4c8
Reviewed-on: http://review.couchbase.org/69365
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21534: test_access_scanner - handle clock wraparound 64/69364/3
Dave Rigby [Tue, 1 Nov 2016 11:40:06 +0000 (11:40 +0000)]
MB-21534: test_access_scanner - handle clock wraparound

test_access_scanner attempts to set the access log scanner runtime to
two hours in the future; however it doesn't check for wraparound
midnight.

Change-Id: I2851331a31773fe32221f3cf266f770f1ebefbaf
Reviewed-on: http://review.couchbase.org/69364
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21320: Notify memcached to visit readyQ and get any ready items 71/69171/7
Manu Dhundi [Mon, 31 Oct 2016 16:53:18 +0000 (09:53 -0700)]
MB-21320: Notify memcached to visit readyQ and get any ready items

We should notify memcached to visit readyQ and get any items that were
pushed there during stream creation. Also, we must notify the memcached
about cursor dropping so that it can visit ep-engine and stream any
pending items and do a subsequent stream state change.

This is not a functional fix. It improves performance however.
It is not absolutely necessary to notify immediately as conn manager
will notify eventually.

Change-Id: Id06fc450a20f6d0258fa7c687520dff5f4899a28
Reviewed-on: http://review.couchbase.org/69171
Reviewed-by: Dave Rigby <daver@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20822: Print correct value for vb_%d:num_erroneous_entries_erased 32/69132/3
Dave Rigby [Mon, 24 Oct 2016 09:21:30 +0000 (10:21 +0100)]
MB-20822: Print correct value for vb_%d:num_erroneous_entries_erased

'num_erroneous_entries_erased' was incorrectly printing the total size
of the failover table, and not the number of elements which have been
removed.

Change-Id: I7bbd75ca4db962cba257a1883ce3635b475287c7
Reviewed-on: http://review.couchbase.org/69132
Reviewed-by: Jim Walker <jim@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21190: update log message for set max_cas 94/68994/5
Jim Walker [Thu, 20 Oct 2016 08:03:40 +0000 (09:03 +0100)]
MB-21190: update log message for set max_cas

There's a future desire to print vbucket's as "vb:"

Change-Id: I5ac58daa01a1dd2ed5a631558bf735c9a81ed821
Reviewed-on: http://review.couchbase.org/68994
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20725: Update drift accounting stats 28/69328/5
Jim Walker [Mon, 31 Oct 2016 12:55:23 +0000 (12:55 +0000)]
MB-20725: Update drift accounting stats

The bucket now reports a total of drift for all VBs which will
allow better tracking of drift (e.g. UI graphing).

Change-Id: I5600df323b32f3f1d11feb5be2cf8ddac5988be9
Reviewed-on: http://review.couchbase.org/69328
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20759: Fix false-positive race on DcpConnMap::numActiveSnoozingBackfills 35/69235/4
Dave Rigby [Wed, 26 Oct 2016 10:12:08 +0000 (11:12 +0100)]
MB-20759: Fix false-positive race on DcpConnMap::numActiveSnoozingBackfills

The existing code was safe, however ThreadSanitizer doesn't know about
our own Spinlocks. Given this shouldn't be a hot path, switch to
normal std:mutex.

ThreadSanitizer report:

WARNING: ThreadSanitizer: data race (pid=23569)
  Read of size 2 at 0x7d840000eca2 by thread T8 (mutexes: write M294, read M27095, write M66205, write M102676, write M95235):
    #0 DcpConnMap::canAddBackfillToActiveQ() ep-engine/src/connmap.cc:1308 (ep.so+0x000000045ac5)
    #1 BackfillManager::schedule() ep-engine/src/dcp/backfill-manager.cc:142 (ep.so+0x00000005b0eb)
    #2 DcpProducer::scheduleBackfillManager() ep-engine/src/dcp/producer.cc:702 (ep.so+0x000000078fe3)
    #3 ActiveStream::scheduleBackfill_UNLOCKED(bool) ep-engine/src/dcp/stream.cc:1016 (ep.so+0x00000008f280)
    #4 ActiveStream::transitionState(stream_state_t) ep-engine/src/dcp/stream.cc:1145 (ep.so+0x000000090589)
    #5 ActiveStream::setActive() ep-engine/src/dcp/stream.h:204 (ep.so+0x00000009958e)
    #6 DcpProducer::streamRequest() ep-engine/src/dcp/producer.cc:327 (ep.so+0x00000007f85d)
    #7 EvpDcpStreamReq ep-engine/src/ep_engine.cc:1573 (ep.so+0x0000000cea78)
    #8 dcp_stream_req_executor memcached/daemon/mcbp_executors.cc:2272 (memcached+0x00000045925c)
    #9 process_bin_packet memcached/daemon/mcbp_executors.cc:4650 (memcached+0x00000046481d)
    #10 mcbp_complete_nread(McbpConnection*) memcached/daemon/mcbp_executors.cc:4759 (memcached+0x00000046481d)
    #11 conn_nread(McbpConnection*) memcached/daemon/statemachine_mcbp.cc:314 (memcached+0x000000472678)
    #12 McbpStateMachine::execute(McbpConnection&) memcached/daemon/statemachine_mcbp.h:43 (memcached+0x000000447054)
    #13 McbpConnection::runStateMachinery() memcached/daemon/connection_mcbp.cc:1003 (memcached+0x000000447054)
    #14 McbpConnection::runEventLoop(short) memcached/daemon/connection_mcbp.cc:1274 (memcached+0x0000004470dd)
    #15 run_event_loop memcached/daemon/connections.cc:147 (memcached+0x00000044b9e9)
    #16 event_handler(int, short, void*) memcached/daemon/memcached.cc:851 (memcached+0x00000041466c)
    #17 event_persist_closure src/libevent/event.c:1319 (libevent_core-2.0.so.5+0x00000000b6b7)
    #18 event_process_active_single_queue src/libevent/event.c:1363 (libevent_core-2.0.so.5+0x00000000b6b7)
    #19 event_process_active src/libevent/event.c:1438 (libevent_core-2.0.so.5+0x00000000b6b7)
    #20 event_base_loop src/libevent/event.c:1639 (libevent_core-2.0.so.5+0x00000000b6b7)
    #21 CouchbaseThread::run() platform/src/cb_pthreads.cc:54 (libplatform.so.0.1.0+0x0000000057a5)
    #22 platform_thread_wrap platform/src/cb_pthreads.cc:66 (libplatform.so.0.1.0+0x0000000057a5)

  Previous write of size 2 at 0x7d840000eca2 by thread T55:
    #0 DcpConnMap::decrNumActiveSnoozingBackfills() ep-engine/src/connmap.cc:1319 (ep.so+0x000000045b7b)
    #1 BackfillManager::backfill() ep-engine/src/dcp/backfill-manager.cc:273 (ep.so+0x00000005a783)
    #2 BackfillManagerTask::run() ep-engine/src/dcp/backfill-manager.cc:62 (ep.so+0x00000005ac1c)
    #3 ExecutorThread::run() ep-engine/src/executorthread.cc:115 (ep.so+0x000000108d96)
    #4 launch_executor_thread ep-engine/src/executorthread.cc:33 (ep.so+0x000000109675)
    #5 CouchbaseThread::run() platform/src/cb_pthreads.cc:54 (libplatform.so.0.1.0+0x0000000057a5)
    #6 platform_thread_wrap platform/src/cb_pthreads.cc:66 (libplatform.so.0.1.0+0x0000000057a5)

Change-Id: I88df57b268c1e615b7c5d2b7caf5f038a53692ba
Reviewed-on: http://review.couchbase.org/69235
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-19672: Fix intermittent failure of 'test producer stream request (partial)' 33/69233/5
Dave Rigby [Wed, 26 Oct 2016 09:48:33 +0000 (10:48 +0100)]
MB-19672: Fix intermittent failure of 'test producer stream request (partial)'

This test was making incorrect assumptions about how / when
checkpoints would be created - it wasn't taking the checkpoint period
(how often we forcefully create new checkpoints) into account in it's
assuptions of checkpoint layout. As such it would occasionally fail if
it took longer than expected to run.

Fix this by essentially disabling chk_period (setting it to an
arbitrarily large value). At the same time make the tests expectations
explicit and check them where possible.

Change-Id: I9944f19d41a0a33064c9c43c8673c7ef9c4a3ab9
Reviewed-on: http://review.couchbase.org/69233
Reviewed-by: Manu Dhundi <manu@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-21109: Fix intermittent failure in 'temp item deletion' 37/69237/4
Dave Rigby [Wed, 26 Oct 2016 12:05:03 +0000 (13:05 +0100)]
MB-21109: Fix intermittent failure in 'temp item deletion'

Test suffers from intermittent failures due to it not accounting for
background fetch of deleted items. The issue is that the read of
curr_temp_items races with the background fetcher - if the background
fetcher has not completed then it will have a value of 1 (expected),
however if the BG fetcher completes before the stat is read then 0
will be returned (as the temp item has been cleaned up).

To address this, disable the reader threads before triggering the
background fetch. This requires that we manually handle the
EWOULDBLOCK that get_meta returns (otherwise we'll hang forever
waiting for the BG fetch task to run).

Change-Id: If1bca36a6517909259b4e3767fd58a7ff99944c3
Reviewed-on: http://review.couchbase.org/69237
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [18/N]: Remove now dead VBucket persist Tasks 51/69151/9
Dave Rigby [Mon, 24 Oct 2016 11:46:01 +0000 (12:46 +0100)]
MB-20852 [18/N]: Remove now dead VBucket persist Tasks

Delete all tasks (apart from Flusher) which used to persist VBucket
state to disk - the Flusher Task now handles all VBucket state
persistence.

Also remove statistics which are no longer valid / relevent -
snapshotVbucketHisto / persistVBStateHisto - we no longer snapshot
separately from flushing.

Change-Id: Ia1b3f30f0f792a8ca739d377443f909138dcaa86
Reviewed-on: http://review.couchbase.org/69151
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [17/N]: Serialize VB state changes 50/69150/11
Dave Rigby [Wed, 21 Sep 2016 15:03:47 +0000 (16:03 +0100)]
MB-20852 [17/N]: Serialize VB state changes

Background/Problem:

MB-20852 exposed an issue with how VBucket state was persisted to disk
- specifically that state can be persisted to disk out of order, and
intermediate state changes could be dropped (not persisted at all).

These problems are caused by the asynchronous tasks which are
responsible for persisting state to disk -
VBStatePersistTask{Low,High}. There are essentially two interrelated
issues:

1. We allow multiple tasks which persist VBState to exist concurrently
   (VBStatePersistTask{Low,High}, Flusher,
   VBSnapshotTask{Low,High}. Furthermore, multiple instances of the
   same task (e.g. VBStatePersistTask) can also exist concurrenlty.

2. We do not maintain a queue of states to persist, instead we just
   mark that a given vbid's state is pending.

(1) Can occur when a VBPersistTask has started to run on a BG writer
    thread (and has cleared the schedule_vbstate_persist flag), but
    then another scheduleVBStatePersist() call is made. As the
    schedule_vbstate_persist flag is clear, this second task is
    allowed to be created. There is then no guarantee which task will
    complete first (they could be scheduled to different OS writer
    threads, the first thread could be descheduled and the second one
    then runs and completes first).

    We could attempt to solve this by changing when
    schedule_vbstate_persist is cleared (say move it later in the
    persistVBState() function), but then the inverse problem is
    exposed - we may fail to schedule a second (different) state to be
    persisted if the current task is just finishing up (and will exit
    without persisting the now-outstanding work).

(2) Presents a subtle problem relating to when the state of a VBucket
    is materialized. As we only record the vbid to persist (and not
    the state), by the time the VBucketPersistTask runs the actual
    state we /wanted/ to write may have moved forward. Even worse, the
    state could have "gone backwards" (as shown in the MB) if the
    state of the VBucket is read before the vbucket is deleted,
    meaning the task has a 'stale' view of the VBucket object (due to
    us using ref-counted pointers for VBucket objects).

    Additionally, not persisting all intermediate states makes
    debugging harder. We don't actually change VBucket state /that/
    often, and so having all the intermediate states a VBucket went
    through on disk is extremely valuable in debugging.

Solution:

Instead of having multiple different tasks which can persist state
(and attempting to manage when they are created / when they run / what
state they persist), we instead use a single task (the Flusher task)
to persist state for a given vBucket. Note the Flusher *already*
persists vbucket state if necessary during commit (see
EventuallyPersistentStore::flushVBucket), so this path just adopts the
Flusher as the canonical Task to perform vbstate persistence.

To ensure that state is persisted even when there are no outstanding
'normal' items in the vbucket's checkpoint queue, we use the new
queue_op::set_vbucket_state meta-item to signify that a persist is
pending (and to essentially make the pending items queue non-empty so
the flusher will run).

After this patch all the other Tasks which used to persist vbucket
state are redundent - a subsequent patch will remove them.

Change-Id: Ic44360a1dd14fb882ebfa6f28f4ccfe6127d17a8
Reviewed-on: http://review.couchbase.org/69150
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [16/N]: Add queue_op::set_vbucket_state meta-item 49/69149/7
Dave Rigby [Mon, 24 Oct 2016 11:01:15 +0000 (12:01 +0100)]
MB-20852 [16/N]: Add queue_op::set_vbucket_state meta-item

Add a new meta-item to queue_op enum - set_vbucket_state. This will be
used to mark that a VBucket should have it's state persisted.

Change-Id: I108befd70933962d262529ee230382e47c64e31a
Reviewed-on: http://review.couchbase.org/69149
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [15/N]: Accurately track meta items within checkpoints 20/69020/8
Dave Rigby [Tue, 11 Oct 2016 16:14:03 +0000 (17:14 +0100)]
MB-20852 [15/N]: Accurately track meta items within checkpoints

Instead of assuming that a Checkpoint only contains 1 (Open) or 2
(Closed) meta-items, maintain a count of items within each Checkpoint,
and track how many meta-items a CheckpointCursor has read.

This allows us to support an arbitrary number of meta-items within a
Checkpoint, and in any sequence. This feature will be used to add
support for set_vbstate meta-items in a subsquent patches.

Change-Id: I8fb3040cbe64e316aae1f693afee65001b2b4b17
Reviewed-on: http://review.couchbase.org/69020
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [14/N]: Improve debug/logging in CheckpointManager 21/69021/8
Dave Rigby [Thu, 20 Oct 2016 13:58:01 +0000 (14:58 +0100)]
MB-20852 [14/N]: Improve debug/logging in CheckpointManager

Include information on the CheckpointCursors associated with a
CheckpointManager by adding a operator<< for CheckpointCursor. Add a
dump() method to CheckpointManager to assist in debugging their
contents (e.g. from gdb).

Also add some additional CheckpointManager/CouchKVStore logging.

Change-Id: I8c3de5b5ec0e8e297db8530dee87ac0edd869a91
Reviewed-on: http://review.couchbase.org/69021
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [13/N]: Checkpoint: Add getNumMetaItems() method 18/69018/8
Dave Rigby [Thu, 13 Oct 2016 16:08:53 +0000 (17:08 +0100)]
MB-20852 [13/N]: Checkpoint: Add getNumMetaItems() method

Add a new method to Checkpoint which returns the number of metaItems
in the checkpoint.

Initially this just returns a fixed value of 1 if the checkpoint is
open, or 2 if closed, as this matches the current checkpoint meta item
usage.

However subsequent patches will modify this to track the count of how
many meta items actually are in the checkpoint and hence allow us to
determine the meta item count when n queue_op::set_vbstate is added.

Change-Id: I411f2e97e16f9b11ac19a1b7165e4767d09f37d1
Reviewed-on: http://review.couchbase.org/69018
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [12/N]: Add VBucket::getVBucketState method, use vector for VBuckets in Map 89/67889/12
Dave Rigby [Wed, 21 Sep 2016 14:38:47 +0000 (15:38 +0100)]
MB-20852 [12/N]: Add VBucket::getVBucketState method, use vector for VBuckets in Map

Add a method which will encode the current state of the VBucket into a
vbucket_state struct.

Move from a raw array new to std::vector for VBuckets.

Change-Id: I4755568f977b6e97e7ed9e3bdc64a76d842b6ebd
Reviewed-on: http://review.couchbase.org/67889
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [11/N]: Move persistenceCheckpoint id to VBucket 77/67877/14
Dave Rigby [Tue, 13 Sep 2016 16:22:38 +0000 (17:22 +0100)]
MB-20852 [11/N]: Move persistenceCheckpoint id to VBucket

Currently the VBucketMap owns the persistence checkpoint IDs of all
VBuckets. Given these are properties of the VBuckets themselves (for
example one cannot form a vbucket_state record without one) it seems
more logical to have the VBucket class itself own this property.

Change-Id: I50260da628f543365081e736fdd55056a6004cd9
Reviewed-on: http://review.couchbase.org/67877
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [10/N]: Return by value from VBucket::getPersistedSnapshot 75/68275/8
Dave Rigby [Mon, 3 Oct 2016 09:23:39 +0000 (10:23 +0100)]
MB-20852 [10/N]: Return by value from VBucket::getPersistedSnapshot

Simplifies usage & makes it explicit that all members of the range are
returned.

Change-Id: I43159f7dc4836b4d30c98d0e07bf570f3f34cc1e
Reviewed-on: http://review.couchbase.org/68275
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agoMB-20852 [9/N]: Explicilty handle all queue_op uses 61/68161/8
Dave Rigby [Thu, 29 Sep 2016 14:05:14 +0000 (15:05 +0100)]
MB-20852 [9/N]: Explicilty handle all queue_op uses

Remove instances of switch statements where we don't check all
enumerations in queue_op, and instead handle them all. Makes it easier
/ safer to add any new queue_op types (and ensure they are handled
everywhere).

Change-Id: I9406c6d327d572268afa4d1830d690ee8ec153a1
Reviewed-on: http://review.couchbase.org/68161
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [8/N]: Improve documentation of putCursorsInCollapsedChk 17/69017/5
Dave Rigby [Tue, 11 Oct 2016 14:12:03 +0000 (15:12 +0100)]
MB-20852 [8/N]: Improve documentation of putCursorsInCollapsedChk

Change-Id: Ibe29d7507aea0f49593454416e9e58e4994ffa4e
Reviewed-on: http://review.couchbase.org/69017
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [7/N]: CheckpointManager::queueDirty: Pass vb by reference 16/69016/5
Dave Rigby [Wed, 19 Oct 2016 10:26:33 +0000 (11:26 +0100)]
MB-20852 [7/N]: CheckpointManager::queueDirty: Pass vb by reference

The VBucket argument to queueDirty is passed via a (reference to) a
ref-counted pointer. Nothing in this function modifies the arguments'
ref count, or passes it another owner; moreover it is also never null.

Therefore change to pass as a reference.

See also:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-smartptrparam

Change-Id: Iccfeb42922da558b6e9ab430b96829002e85af4a
Reviewed-on: http://review.couchbase.org/69016
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [6/N]: Simplify {start,stop}Flusher & flushVBucket, move to C++11 90/67890/8
Dave Rigby [Thu, 29 Sep 2016 14:24:38 +0000 (15:24 +0100)]
MB-20852 [6/N]: Simplify {start,stop}Flusher & flushVBucket, move to C++11

Move to more C++11 style, and add some comments as to what certain
members mean.

Change-Id: Ibdb900cfe63a9b1352fa1020f9dc58839e3e446d
Reviewed-on: http://review.couchbase.org/67890
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
4 years agoMB-20852 [5/N]: Checkpoint: C++11-ification 15/69015/6
Dave Rigby [Tue, 11 Oct 2016 12:37:10 +0000 (13:37 +0100)]
MB-20852 [5/N]: Checkpoint: C++11-ification

Convert more of the checkpoint code to C++11 - mostly using auto for
long type names.

Change-Id: I16138f6b16f72e065ecc2c82d973caae7786f846
Reviewed-on: http://review.couchbase.org/69015
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agoMB-20852 [4/N]: Use named struct when moving cursors between checkpoints 14/69014/7
Dave Rigby [Tue, 11 Oct 2016 12:33:38 +0000 (13:33 +0100)]
MB-20852 [4/N]: Use named struct when moving cursors between checkpoints

Instead of an anonymous std::pair (which is hard to follow what the
two fields are), use a named struct `CursorPosition` and related
CursorIdToPositionMap map when recording cursor positions to move
between checkpoints.

Change-Id: I39d14dbdae34cd20fe880e23b95d01c5fbfdac91
Reviewed-on: http://review.couchbase.org/69014
Reviewed-by: Jim Walker <jim@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [3/N]: checkpoint_test enhancements 13/69013/6
Dave Rigby [Mon, 10 Oct 2016 13:44:44 +0000 (14:44 +0100)]
MB-20852 [3/N]: checkpoint_test enhancements

Tighten up existing checkpoint tests by adding additional EXPECT
checks.

Add a new CursorMovementReplicaMerge test which tests merging of
cursors on a replica VBucket.

Change-Id: Ia46812c2cd61eeea821a4b2ea46f645e1c66cf7e
Reviewed-on: http://review.couchbase.org/69013
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agoMB-20852 [2/N]: Convert queue_operation to scoped enum 60/68160/12
Dave Rigby [Thu, 29 Sep 2016 13:52:16 +0000 (14:52 +0100)]
MB-20852 [2/N]: Convert queue_operation to scoped enum

In preparation for adding new queue_op for setVBucketState, convert to
a typesafe C++11 scoped enum. Improve the documentation around
queue_op, and related classes Checkpoint, CheckpointCursor.

Also improve the output streaming (operator<<) for Checkpoint class.

Change-Id: I8f29b8e9e8915a68e31550b78bf3131b3737e2d7
Reviewed-on: http://review.couchbase.org/68160
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
4 years agoMB-20852 [1/N]: Update tests to facilitate set_vbucket_state changes 10/69010/10
Dave Rigby [Wed, 19 Oct 2016 11:58:11 +0000 (12:58 +0100)]
MB-20852 [1/N]: Update tests to facilitate set_vbucket_state changes

Use checkXX() macros instead of cb_assert() in a number of test which
check for number of items enqueued.

Also adjust the tests to find the initial value of ep_total_enqueued
before storing items, and use this when checking for expected
values. This facilitates the tests working once the changes for
MB-20852 add the set_vbucket_state meta-item to disk queues.

Change-Id: I0108cc5b635b5376d5852b513a6ed47e67f1e62b
Reviewed-on: http://review.couchbase.org/69010
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agoAdd error checking to test_access_scanner 12/69012/6
Dave Rigby [Mon, 17 Oct 2016 13:32:26 +0000 (14:32 +0100)]
Add error checking to test_access_scanner

test_access_scanner doesn't check if store() has actually succeeded
when populating data. Fix this.

Change-Id: Ibd4f97b3b47c15b81a7671d5ed32c15aa4796a40
Reviewed-on: http://review.couchbase.org/69012
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>
4 years agowait_for_flusher_to_settle: Use wait_for_stat_to_be() 11/69011/5
Dave Rigby [Fri, 14 Oct 2016 16:04:34 +0000 (17:04 +0100)]
wait_for_flusher_to_settle: Use wait_for_stat_to_be()

wait_for_stat_to_be() records the last value (in current) which makes
it easier to debug issues when values arn't as expected. Additionally
this simplifies the code in wait_for_flusher_to_settle.

Change-Id: I601ab4e8b605c937a86b802746cad691ac659308
Reviewed-on: http://review.couchbase.org/69011
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Trond Norbye <trond.norbye@gmail.com>