Namespace retention configurability and dynamic namespace registry #306

prateek · 2017-06-19T15:44:52Z

Prior to this PR, RetentionOptions were global across namespaces and the commit log. This PR changes that to allow every namespace, and the database commit log to have independent retention options.

Major changes:

namespace.Options has a new field: RetentionOptions
New construct: namespace.NamespaceRegistry (maps namespace ID -> namespace Metadata)
Replace RetentionOptions with NamespaceRegistry in all Options
Corresponding changes to retrieve retentionOpts based on Namespace everywhere (e.g. a couple of client.AdminClient method, databaseFlushManager, databaseCleanupManager, etc).
- this induced a lot of interface changes, especially in tests, to pass the ns id parameter.

/cc @robskillington @xichen2020 @martin-mao @cw9

Pending chunks:

prateek · 2017-06-21T21:08:16Z

Makefile

@@ -111,7 +111,7 @@ lint:

 test-internal:
 	@which go-junit-report > /dev/null || go get -u github.com/sectioneight/go-junit-report
-	@$(VENDOR_ENV) $(test) $(coverfile) | tee $(test_log)
+	@$(VENDOR_ENV) NPROC=1 $(test) $(coverfile) | tee $(test_log)


TODO: undo this once builds start working

coveralls · 2017-09-05T21:29:12Z

Coverage increased (+0.8%) to 78.279% when pulling fa5ee9e on prateek/ns-isolation into e19bc7a on master.

coveralls · 2017-09-07T21:02:37Z

Coverage increased (+0.9%) to 78.37% when pulling ec067be on prateek/ns-isolation into e19bc7a on master.

coveralls · 2017-09-09T22:50:32Z

Coverage increased (+0.9%) to 78.375% when pulling aa7bbd0 on prateek/ns-isolation into e19bc7a on master.

robskillington · 2017-09-11T14:37:48Z

integration/commitlog_bootstrap_multi_ns_test.go

+func TestCommitLogBootstrapMultipleNamespaces(t *testing.T) {
+	if testing.Short() {
+		t.SkipNow() // Just skip if we're doing a short run
+


nit: Superfluous endline here.

robskillington · 2017-09-11T14:44:57Z

persist/fs/commitlog/options.go

+		return fmt.Errorf("invalid commit log retention buffer future, expected 0, observed: %v", v)
+	}
+
+	if v := ropts.BufferPast(); v != 0 {


If this is the expectation for buffer future and buffer past, wouldn't it be better to simply have BlockSize as its own option instead of having RetentionOptions be an option?

I was avoiding that cause I needed both BlockSize and RetentionPeriod. But you're right, it's probably cleaner to have it that way. Will address in next patch

robskillington · 2017-09-11T14:53:35Z

storage/cleanup.go

+		flushStart = retention.FlushTimeStart(ropts, t)
+		flushEnd   = retention.FlushTimeEnd(ropts, t)
+	)
+	return flushStart.Add(-blockSize), flushEnd.Add(-blockSize)


Ok sounds good, I guess its because you never want to cleanup a block before the current flush end potential time. I'm fine to leave this as is.

robskillington · 2017-09-11T14:55:19Z

storage/cleanup.go

+	// If we cared about 100% correctness, we would optimize this by retaining a smarter data
+	// structure (e.g. interval tree), but for our use-case, it's safe to assume that commit logs
+	// are only retained for a period of 1-2 days (at most), after we which we'd live we with the
+	// data loss.


Ok sounds good.

The other thing is if you turn off the process for however long the commit log retention time is, then turn it back on, they'll never get cleaned up. Which is a bit poor.

robskillington · 2017-09-11T14:59:51Z

storage/database.go

-	d.mediator = mediator
+
+	// wait till first value is received
+	<-watch.C()


For debuggability perhaps log the start and the stop of this operation?

i.e.

log.Info("resolving namespaces with namespace watch") <-watch.C() for _, namespace := range watch.Get() { log.Infof("resolved namespace: %s", namespace.ID().String()) }

Potentially we might want to do the same for the cluster/database.go startup routine too.

I'll add the line ~ log.Info("resolving namespaces with namespace watch"), I don't think we need the loop/log ~ it's already done in UpdateOwnedNamespaces.

Example log from startup of the node

Sep 7 22:33:22 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"level":"info","msg":"m3dbnode_main.go:815 bytes pool registering bucket capacity=8192, count=32768","time":"2017-09-07T22:33:22Z"} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"level":"info","msg":"m3dbnode_main.go:492 creating config service client with m3kv","time":"2017-09-07T22:33:56Z"} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"level":"info","msg":"log.go:50 m3db: adding a watch for service: statsdex_m3dbnode env: production zone: sjc1 includeUnhealthy: true","time":"2017-09-07T22:33:56Z"} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"level":"info","msg":"log.go:50 m3db: successfully loaded cache from file /var/lib/m3kv/statsdex_m3dbnode_sjc1.json","time":"2017-09-07T22:33:56Z"} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"level":"info","msg":"log.go:50 m3db: successfully loaded cache from file /var/lib/m3kv/_kv_production_statsdex_m3dbnode_sjc1.json","time":"2017-09-07T22:33:56Z"} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"key":"m3db.node.bootstrapper","level":"info","msg":"m3db: value update success","time":"2017-09-07T22:33:56Z","value":["filesystem","peers","noop-all"],"version":81} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"adds":"testmetrics, metrics","level":"info","msg":"m3db: updating database namespaces","removals":"[]","time":"2017-09-07T22:33:56Z","updates":"[]"} Sep 7 22:33:56 m3-tsdb215-sjc1 statsdex_m3dbnode[26210]: {"level":"info","msg":"m3dbnode_main.go:624 node tchannelthrift: listening on 0.0.0.0:6155","time":"2017-09-07T22:33:56Z"}

added appropriate logs to storage/database.go, storage/cluster/database.go, and storage/namespace_watch.go

robskillington · 2017-09-11T15:06:51Z

storage/database.go

+	if len(t) == 0 {
+		return "[]", nil
+	}
+	var buf bytes.Buffer


nit: Considering you return [] if len(t) == 0 perhaps would be best to prefix and suffix if you have real values with [ and ]? i.e. buf.WriteRune('[') at beginning and buf.WriteRune(']') at end?

robskillington · 2017-09-11T15:07:17Z

storage/database.go

+	if len(m) == 0 {
+		return "[]", nil
+	}
+	var buf bytes.Buffer


Same here regarding prefix and postfix with [ and ].

robskillington · 2017-09-11T15:08:27Z

storage/flush.go

+	if err != nil {
+		return err
+	}
+	defer flush.Done()


Does flush.Done() return an error? If so should it be checked? And if not shouldn't it return an error considering its an IO based operation that could encounter an error?

Good catch, this was following the original code. Will update to capture that error, and add a test case for it.

Found the same thing in checkpoint file write method ~ persist/fs/write.go. Fixed in #347.

Opened #346 as a followup.

…isation (#338)

coveralls · 2017-09-11T21:24:27Z

Coverage increased (+0.9%) to 78.403% when pulling c1babe8 on prateek/ns-isolation into e19bc7a on master.

robskillington · 2017-09-12T21:53:29Z

storage/namespace/dynamic/options.go

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


Any reason why DynamicOptions is defined in the namespace package and not directly as Options in the dynamic package - which would be more consistent with other packages?

I suppose to be as similar as possible to the topology package all the contents of the dynamic package would just be in namespace and just prefixed with Dynamic e.g. NewDynamicInitializer(...) namespace.Initializer etc.

Sure, I'll move static/* and dynamic/* into the namespace package

robskillington · 2017-09-12T21:59:15Z

storage/namespace/map.go

+		namespaces: ns,
+		ids:        ids,
+		metadatas:  nsMetadatas,
+	}, multiErr.FinalError()


nit: Hm just to be more Go-ish maybe better to if err := multiErr.FinalError(); err != nil { return } before return the *nsMap result? Otherwise its the not the typical one or the other return. I like that this avoids branching (for higher levels of test coverage) but I feel sticking to convention may be better.

robskillington · 2017-09-12T22:07:36Z

storage/repair.go

 		}
+		r.repairStatesByNs.setRepairState(n.ID(), t, repairState)


Haven't quite grokked the locking for the dbRepairer but thought should check in, does this need to be locked when get/set the repair states? Doesn't seem like callers of this method necessarily lock.

Yep we don't lock because the databaseRepairer guarantees at most 1 repair is executing

Does that mean we can remove all the other locks? Perhaps we can document at the struct level which fields are protected by mutex and which ones protected by the run lock?

Yep can move the lock to only lock a single field, will make the change.

robskillington

LGTM

coveralls · 2017-09-13T01:31:12Z

Coverage increased (+0.9%) to 78.376% when pulling 2d9d1d4 on prateek/ns-isolation into e19bc7a on master.

coveralls · 2017-09-13T01:52:13Z

Coverage increased (+0.9%) to 78.346% when pulling 5ead7ac on prateek/ns-isolation into e19bc7a on master.

prateek force-pushed the prateek/ns-isolation branch from 363f619 to c7155ae Compare June 19, 2017 18:09

m3db deleted a comment from coveralls Jun 19, 2017

m3db deleted a comment from coveralls Jun 20, 2017

prateek force-pushed the prateek/ns-isolation branch from 9b6e711 to 37dd13b Compare June 20, 2017 15:30

m3db deleted a comment from coveralls Jun 20, 2017

prateek commented Jun 21, 2017

View reviewed changes

m3db deleted a comment from coveralls Jun 22, 2017

m3db deleted a comment from coveralls Jun 23, 2017

prateek added 2 commits June 23, 2017 22:03

negative test for cleanup

509ffb5

multi namespace disk flush integration test

eb8ee1e

m3db deleted a comment from coveralls Jun 24, 2017

try parallelizing travis builds

60609cf

m3db deleted a comment from coveralls Jun 24, 2017

prateek added 2 commits June 24, 2017 01:08

splitting tests further

855f7ca

trying with parallelism at 2

8c8752e

m3db deleted a comment from coveralls Jun 24, 2017

prateek mentioned this pull request Sep 7, 2017

Avoid redundant series pool creation #342

Merged

Avoid redundant series pool creation (#342)

ec067be

Migrate read/write/fetch metrics to be per Ns (#337)

aa7bbd0

robskillington reviewed Sep 11, 2017

View reviewed changes

prateek mentioned this pull request Sep 11, 2017

Enable ErrCheck during metalinting #346

Closed

Prateek Rungta and others added 4 commits September 11, 2017 15:02

Address feedback

cc10b50

fix metalint issues

a7575a8

Check checkpoint file writer error (#347)

b8df2ae

Increase test coverage for namespace kv config serialisation/deserial…

c1babe8

…isation (#338)

m3db deleted a comment from coveralls Sep 11, 2017

robskillington reviewed Sep 12, 2017

View reviewed changes

Address feedback

7041802

robskillington approved these changes Sep 13, 2017

View reviewed changes

prateek and others added 3 commits September 12, 2017 20:51

Add nil namespace key propagation check (#349)

c4b655a

increase delete ns integration test timeout to satisfy travis

2d9d1d4

cleanup pending todos

5ead7ac

m3db deleted a comment from coveralls Sep 13, 2017

prateek merged commit e3ca7a3 into master Sep 13, 2017

prateek deleted the prateek/ns-isolation branch January 15, 2018 15:41

robskillington changed the title ~~Improve Namespace Isolation~~ Namespace retention configurability and dynamic namespace registry Jan 24, 2018

Namespace retention configurability and dynamic namespace registry #306

Namespace retention configurability and dynamic namespace registry #306

Conversation

prateek commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Sep 5, 2017 • edited Loading

coveralls commented Sep 7, 2017 • edited Loading

coveralls commented Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Sep 11, 2017

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

robskillington left a comment

Choose a reason for hiding this comment

coveralls commented Sep 13, 2017

coveralls commented Sep 13, 2017 • edited Loading

prateek commented Jun 19, 2017 •

edited

Loading

coveralls commented Sep 5, 2017 •

edited

Loading

coveralls commented Sep 7, 2017 •

edited

Loading

coveralls commented Sep 9, 2017 •

edited

Loading

prateek Sep 11, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading