Add support for putting commit log bootstrapper before peers #894

richardartoul · 2018-09-10T16:51:13Z

Add a new "uninitialized" bootstrapper which can be added to the end of the bootstrapping process and will only succeed bootstraps for uninitialized topologies
Add logic to the commit log bootstrapper to only announce that it can succeed a bootstrap if the requested shard is marked as "Available" for itself in the topology
General refactoring for code sharing
Update example config files to use bootstrappers: filesystem,commitlog,peers,uninitialized

I also tested the following flows on the test cluster (with bootstrapping configuration filesystem,commitlog,peers,uninitialized):

Turn off all nodes, create new topology, create new namespace, turn on all nodes and make sure they're able to start
Do a node remove and make sure bootstrap succeeds
Do a node add and make sure bootstrap succeeds
Roll the entire cluster and make sure each node is able to bootstrap

We will need to address #900 in a separate P.R

codecov · 2018-09-10T20:11:12Z

Codecov Report

Merging #894 into master will increase coverage by <.01%.
The diff coverage is 67.53%.

@@            Coverage Diff             @@
##           master     #894      +/-   ##
==========================================
+ Coverage   78.67%   78.67%   +<.01%     
==========================================
  Files         396      399       +3     
  Lines       33533    33678     +145     
==========================================
+ Hits        26382    26497     +115     
- Misses       5354     5372      +18     
- Partials     1797     1809      +12

Flag	Coverage Δ
#dbnode	`81.41% <67.53%> (-0.03%)`	⬇️
#m3ninx	`71.93% <ø> (ø)`	⬆️
#query	`69.55% <ø> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2883443...4ac6997. Read the comment docs.

robskillington · 2018-09-11T17:54:03Z

kube/bundle.yaml

@@ -176,6 +176,7 @@ data:
        bootstrappers:
            - filesystem
            - commitlog
+            - uninitialized


Ah much better name 👍

Do we want to be putting peers after commitlog in all these configurations?

haha I thought you wouldn't like the name but I couldn't think of anything better, and yeah sure why not. It should all work now

robskillington · 2018-09-11T17:56:08Z

src/cmd/services/m3dbnode/config/bootstrap.go

@@ -195,6 +204,12 @@ func ValidateBootstrappersOrder(names []string) error {
 			bfs.FileSystemBootstrapperName,
 			peers.PeersBootstrapperName,
 		},
+		uninitialized.UninitializedBootstrapperName: []string{


Looks good. For peers bootstrapper, shouldn't the commit log bootstrapper name also appear in there (so it can appear after the commit log bootstrapper as well)?

good call, will fix

robskillington · 2018-09-11T18:01:08Z

src/dbnode/config/m3dbnode-local-etcd.yml

@@ -63,7 +63,7 @@ db:
    bootstrappers:
        - filesystem
        - commitlog
-        - noop-none
+        - uninitialized


Hm as before, do we want to be putting peers after commitlog in all these configurations?

Also very surprised that we don't have peers actually listed here by default.

yeah sure thing. Probably because it was too confusing to explain it to people haha

…hing if node is Available or Leaving for that shard and add uninitialized source

robskillington · 2018-09-12T19:32:21Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go

+		// In the Initializing and Unknown states we have to assume that the commit log
+		// is missing data and can't satisfy the bootstrap request.
+		case shard.Initializing:
+		case shard.Unknown:


Should shard.Unknown be removed here perhaps and let this case drop through to the default case?

yeah thats fair

robskillington · 2018-09-12T19:36:09Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go

+			iOpts := s.opts.CommitLogOptions().InstrumentOptions()
+			invariantLogger := instrument.EmitInvariantViolationAndGetLogger(iOpts)
+			invariantLogger.Errorf(
+				"Initial topology state does not contain shard state for origin node and shard: %d", shardIDUint)


nit: Lowercase the first letter of the log message for consistency?

robskillington · 2018-09-12T19:46:20Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/source.go

+			switch shardState {
+			case shard.Initializing:
+				numInitializing++
+			case shard.Unknown:


I don't think we should track Unknown either, it should never be a part of a topology.

I'll do the same fallthrough thing

prateek · 2018-09-13T18:42:11Z

src/dbnode/storage/bootstrap/result/shard_ranges.go

 // unfufilled time ranges from the set of shard time ranges.
-func (r ShardTimeRanges) ToUnfulfilledResult() DataBootstrapResult {
+func (r ShardTimeRanges) ToUnfulfilledDataResult() DataBootstrapResult {


prateek · 2018-09-13T18:45:47Z

src/dbnode/topology/testutil/topology.go

@@ -101,3 +106,41 @@ func (v TopologyView) Map() (topology.Map, error) {

 	return topology.NewStaticMap(opts), nil
 }
+
+// SourceAvailableHost is a human-friendly way of constructing


suggestion(take/leave): i constructed topologies using something like --

func TestFetchTaggedResultsAccumulatorAnyResponseShouldTerminateConsistencyLevelOneSimpleTopo(t *testing.T) { // rf=3, 30 shards total; three identical hosts topoMap := tu.MustNewTopologyMap(3, map[string][]shard.Shard{ "testhost0": tu.ShardsRange(0, 29, shard.Available), "testhost1": tu.ShardsRange(0, 29, shard.Available), "testhost2": tu.ShardsRange(0, 29, shard.Available), })

I think something equivalent tu.MustNewStateSnapshot(...) might read better when using than this method.

Yeah this looks good, at this point I'd be re-writing a bunch of tests for a fairly superficial change (tests other than the ones I'm modifying in this P.R) so I'm gonna leave for now

More than the superficial, it's about keeping the tests simpler to read/maintain. If you wanna do it in another PR, mind opening an issue

prateek · 2018-09-13T18:46:30Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/types.go

+	SetInstrumentOptions(value instrument.Options) Options
+
+	// Validate validates the options are correct.
+	Validate() error


nit: convention to have this as the first method in the Options() type.

prateek · 2018-09-13T18:47:24Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/types.go

+	InstrumentOptions() instrument.Options
+
+	// Set the instrument options.
+	SetInstrumentOptions(value instrument.Options) Options


nit: follow ordering of setter before getter for each type.

prateek · 2018-09-13T18:48:04Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/source_test.go

+	"github.com/m3db/m3/src/dbnode/storage/bootstrap"
+	"github.com/m3db/m3/src/dbnode/storage/bootstrap/result"
+	"github.com/m3db/m3/src/dbnode/storage/namespace"
+	topotestutils "github.com/m3db/m3/src/dbnode/topology/testutil"


suggestion: tu instead of topotestutils

prateek · 2018-09-13T18:49:03Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/source.go

+
+// The purpose of the unitializedSource is to succeed bootstraps for any
+// shard/time-ranges if the given shard/namespace combination has never
+// been completely initialized (is a new namespace). This is required for


nit: it's not per namespace, it's per cluster

you're right, good call

prateek · 2018-09-13T18:51:14Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source_prop_test.go

@@ -40,7 +40,9 @@ import (
 	"github.com/m3db/m3/src/dbnode/persist/fs/commitlog"
 	"github.com/m3db/m3/src/dbnode/storage/bootstrap/result"
 	"github.com/m3db/m3/src/dbnode/storage/namespace"
+	topotestutils "github.com/m3db/m3/src/dbnode/topology/testutil"


same as other spot, tu would probably be more concise

prateek · 2018-09-13T18:54:02Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go

@@ -1425,6 +1421,72 @@ func (s commitLogSource) maybeAddToIndex(
 	return err
 }

+func (s *commitLogSource) availability(


nit: mind adding 1-2 lines explaining intent behind the method -- i.e. that anything not initialized is by definition not available

prateek · 2018-09-13T18:55:43Z

src/dbnode/storage/bootstrap/bootstrapper/peers/source.go

 			default:
-				panic(
-					fmt.Sprintf("encountered unknown shard state: %s", shardState.String()))
+				// TODO(rartoul): Make this a hard error once we refactor the interface to support


prateek · 2018-09-13T18:56:57Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/options.go

+
+// NewOptions creates a new Options.
+func NewOptions() Options {
+	return &options{}


hm why not initialise these to result.NewOptions() and instrument.NewOptions()?

asking because almost always NewOptions() returns something that's valid by default

To avoid issues like we've had in the past where we cant find metrics because (somewhere) in the deep configuration pipeline / setup we forgot to set InstrumentOptions()

This seems like a reasonable compromise to make sure that we don't run into that type of stuff anymore.

Don't necessarily disagree but doing it in one place like this breaks from the convention everywhere else in the code.

prateek · 2018-09-13T18:58:38Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/source.go

+		}
+
+		// The basic idea for the algorithm is that on a shard-by-shard basis we
+		// need to determine if the namespace is "new" in the sense that it has


nit: instead of namespace is "new" --> cluster is "new"

Thanks, not sure why I kept making that mistake. I guess its easy to think of the topology as being per-namespace but actually shards are for the whole cluster not a namespace

prateek · 2018-09-13T18:59:05Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/source.go

+		// in the topology are "available").
+		// In order to determine this, we simply count the number of hosts in the
+		// "initializing" state. If this number is larger than zero, than the
+		// namespace is "new".


nit: cluster

prateek · 2018-09-13T19:00:19Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/source.go

+			}
+		}
+
+		shardHasNeverBeenCompletelyInitialized := numInitializing-numLeaving > 0


hm won't this break for a replace?

discussed offline, should be fine for everything except a replication factor change. Will add a comment

prateek · 2018-09-17T18:09:17Z

kube/m3dbnode-configmap.yaml

@@ -68,6 +68,8 @@ data:
        bootstrappers:
            - filesystem
            - commitlog
+            - peers
+            - uninitialized_topology


i hate it, but i hate all others more =P

prateek · 2018-09-17T19:25:38Z

src/dbnode/storage/bootstrap/bootstrapper/uninitialized/options.go

+}
+
+// NewOptions creates a new Options.
+func NewOptions() Options {


Was tracing through uses of this type, don't think you need the resultOptions - it's unused afaict, and the instrumentOptions are only required because of the logger, which you can avoid once the interface change is done to allow error propagation back up. Maybe leave a note to delete the type in the future once it's unused?

Seems like the base bootstrapper uses it for some things in the provider.

prateek

LGTM

robskillington

LGTM

prateek

LGTM

richardartoul force-pushed the ra/commitlog-peers-v2 branch from 3a38f69 to bfb9413 Compare September 10, 2018 20:29

richardartoul changed the title ~~[WIP] - Add support for putting commit log bootstrapper before peers~~ Add support for putting commit log bootstrapper before peers Sep 10, 2018

richardartoul requested review from prateek and robskillington and removed request for prateek September 10, 2018 21:59

robskillington reviewed Sep 11, 2018

View reviewed changes

richardartoul force-pushed the master branch from 2883443 to 2cf04be Compare September 11, 2018 18:16

richardartoul force-pushed the ra/commitlog-peers-v2 branch from b680f94 to 93b0eef Compare September 11, 2018 19:04

Richard Artoul added 14 commits September 11, 2018 15:33

Refactor commit log bootstrapper to only return it can satisfy everyt…

61af15d

…hing if node is Available or Leaving for that shard and add uninitialized source

Improve comments

77fe988

Improve comments

7b1646e

Rename var

bfad99f

Improve comment

11367e1

Refactor and add uninitialized bootstrapper provider

a8ef19e

Fix topology initialization logic

232fce9

Fix lint issues

47c1473

Fix prop test

6f53985

Fix lint issues and rebase

a3d2bf1

Fix broken tests

4e0e180

Fix broken test

a1fe6c2

Update config files

139b87a

Address review feedback

4ac6997

richardartoul force-pushed the ra/commitlog-peers-v2 branch from 93b0eef to 4ac6997 Compare September 11, 2018 19:34

robskillington reviewed Sep 12, 2018

View reviewed changes

prateek reviewed Sep 13, 2018

View reviewed changes

Richard Artoul added 5 commits September 17, 2018 10:24

rename uninitialzied bootstrapper to uninitialized_topology bootstrapper

22b4495

Address review feedback

b48b4e4

Address more review feedback

9c1b059

Fix config to use new bootstrapper name

437df7a

Fix config to use new bootstrapper name

d3bb018

prateek reviewed Sep 17, 2018

View reviewed changes

Rewrite SourceAvailableHosts to use map structure

7777763

richardartoul mentioned this pull request Sep 17, 2018

Refactor SourceAvailableHost topology test util to be easier to read #909

Closed

Set default for uninitialized source options

c27062a

prateek reviewed Sep 17, 2018

View reviewed changes

prateek previously approved these changes Sep 17, 2018

View reviewed changes

Delete unused errors

1d11b84

richardartoul dismissed prateek’s stale review via 1d11b84 September 17, 2018 19:34

robskillington approved these changes Sep 17, 2018

View reviewed changes

prateek approved these changes Sep 17, 2018

View reviewed changes

richardartoul merged commit 93e770d into master Sep 17, 2018

prateek deleted the ra/commitlog-peers-v2 branch September 29, 2018 18:08

Add support for putting commit log bootstrapper before peers #894

Add support for putting commit log bootstrapper before peers #894

Conversation

richardartoul commented Sep 10, 2018 • edited Loading

codecov bot commented Sep 10, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

prateek Sep 17, 2018 • edited by robskillington Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek left a comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

prateek left a comment

Choose a reason for hiding this comment

richardartoul commented Sep 10, 2018 •

edited

Loading

codecov bot commented Sep 10, 2018 •

edited

Loading

prateek Sep 17, 2018 •

edited by robskillington

Loading