Timeout topology watch and return better error messages for missing topology / namespaces

[richardartoul]

We weren't passing a timeout properly to the ServiceClient so:

watch, err := services.Watch(opts.ServiceID(), opts.QueryOptions())

would hang forever if there was no topology.

In addition, in the case where there is no topology or namespace the error message is cryptic (timeout) when it could be slightly more helpful

[prateek]

Need to be b/w compatible when making changes to publicly configurable fields.

• Could you mark the old fields deprecated,
• Add the new field,
• Error if both new and old are set, and pick the set field otherwise

Rob had to do some of this in #890 -

m3/src/query/storage/local/config.go

Lines 90 to 117 in 721ac92

	// StorageMetricsType is the namespace type.
	//
	// Deprecated: Use "Type" field when specifying config instead, it is
	// invalid to use both.
	StorageMetricsType storage.MetricsType `yaml:"storageMetricsType"`
	}
	func (c ClusterStaticNamespaceConfiguration) metricsType() (storage.MetricsType, error) {
	result := storage.DefaultMetricsType
	if c.Type != storage.DefaultMetricsType && c.StorageMetricsType != storage.DefaultMetricsType {
	// Don't allow both to not be default
	return result, errBothNamespaceTypeNewAndDeprecatedFieldsSet
	}
	if c.Type != storage.DefaultMetricsType {
	// New field value set
	return c.Type, nil
	}
	if c.StorageMetricsType != storage.DefaultMetricsType {
	// Deprecated field value set
	return c.StorageMetricsType, nil
	}
	// Both are default
	return result, nil
	}

[richardartoul]

spoke offline, very unlikely anyone ever set these fields are they are undocumented in are YMLs so gonna make the backward incompatible change

[richardartoul]

Spoke offlien again, gonna remove timeouts altogether

[justinjc]

Capitalize the new sentence. Also, include err in the Errorf.

[robskillington]

Hey, do you want to make sure this supports the case if bringing up a node and using it's coordinator endpoint to create a placement?

Before we set an indefinite timeout here we used to basically crash the node before you could create a placement, which was a fairly big issue.

Does this change re-introduce that behavior? (Just wanting to get to the bottom of how to best cover both cases)

[prateek]

@robskillington Yep, we settled on removing the timeout all together for that reason. As long as we put appropriate log messages before waiting in all the places, it should fail the same as it does today. Sound reasonable?

[codecov]

Codecov Report

Merging #926 into master will decrease coverage by 0.04%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #926      +/-   ##
==========================================
- Coverage   77.96%   77.91%   -0.05%     
==========================================
  Files         410      410              
  Lines       34399    34358      -41     
==========================================
- Hits        26818    26770      -48     
- Misses       5734     5741       +7     
  Partials     1847     1847

Flag	Coverage Δ
#dbnode	81.45% <93.33%> (-0.05%)	⬇️
#m3ninx	75.25% <ø> (-0.08%)	⬇️
#query	64.43% <ø> (-0.04%)	⬇️
#x	80.55% <ø> (ø)	⬆️

---

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 835df4d...3482a5a. Read the comment docs.

[cw9]

We finally removed this, noice!

[prateek]

can't just delete these (b/w compatibility and all), need to mark deprecated instead

[richardartoul]

Spoke offline and agreed its ok in this case because it was undocumented config and we're breaking the functionaltiy anyways

[prateek]

LGTM