client-api: Add metadata as field on ServiceDiscovererEvent

[bryce-anderson]

See also #2901 for a counter API proposal.

Motivation:

We want to add meta-data to our ServiceDiscoverer API's
but how to represent them isn't obvious. This is an example
of what it would look like by making meta-data a map-like
concept.

Modifications:

• Add a Map<String, Object> to the ServiceDiscovererEvent
  to be the carrier of meta-data.
• Add some helpers to make defining meta-data a bit more
  type safe.
• Add the notion of endpoint weight and priority using this pattern.

There are pros and cons

Pros:

• Unlike in client-api: Add weight as field on ServiceDiscovererEvent #2901, We don't need to have every implementation of
  ServiceDiscovererEvent know about every type of meta-data. We
  could provided a DelegatingServiceDiscovererEvent, but no existing
  SD implementations will be using it right now.
• Clean separation of the three logical parts of the ServiceDiscovererEvent,
  specifically the (Address, Status, meta-data).

Cons:

• Even first class concepts such as weight feel somewhat
  out of place.
• A lot more code changes to get started.

[mgodave]

My two cents: if properties have to be defined statically within a class to be visible to both a discovery event and a load balancer you might as well make them part of the interface. If they have a default value that makes sense, such as zero in this case, all the better.

[idelpivnitskiy]

Any particular reason for not using ContextMap instead of a Map with a new custom Key?
I opened #2906 in case we can proceed with ContextMap.

[bryce-anderson]

I see two main reasons. The first being that ContextMap is mutable so we can't return a shared instance. That makes it so a default method takes the form

default ContextMap metadata() {
  return new DefaultContextMap();
}

Which seems not great.

The second is that ContextMap is keyed on the Key instance, not it's name, so the produces and consumers of metadata must share the exact same key making a strong API linkage between the two.

[idelpivnitskiy]

1. See my PR Add ContextMaps to let users create various ContextMaps #2906 that adds ContextMaps with various implementations, including empty, singleton and unmodifiable.

2. That's correct, see HttpContextKeys as an example of how a single key as shared. We can have a similar "key holder" class in client-api.

[bryce-anderson]

Talking this over a bit more with @tkountis and @mgodave, the use case for feeding meta-data to the load balancer is pretty nebulous right now. What I want in the short term is priority group information. In the xDS API that is first class on the LocalityLbEndpoints (here) and so makes sense to flatten it and make it first class on the ServiceDiscovererEvent. Weight is first class on LBEndpoint(here).

So for now, I think we should drop this PR and head back toward #2901 for weight and priority and wait and see what we can actually use meta-data for in the LB before adding it to the ServiceDiscovererEvent class.

[idelpivnitskiy]

I'm certain that if we make any specific fields as the first class citizens of ServiceDiscovererEvent class, there will be a lot of friction before the API settles. Metadata as a map is a way to minimize (if not avoid) that friction and it's a way to add more info for the even without the need to go back to ST and redo the API.
Taking into account we should preserve backward compatibility, prematurely promoting some specific fields to public API is a slippery way.

[bryce-anderson]

I'm certain that if we make any specific fields as the first class citizens of ServiceDiscovererEvent class, there will be a lot of friction before the API settles. Metadata as a map is a way to minimize (if not avoid) that friction and it's a way to add more info for the even without the need to go back to ST and redo the API. Taking into account we should preserve backward compatibility, prematurely promoting some specific fields to public API is a slippery way.

Are you arguing for the Map<String, Object> metadata? Just as @mgodave noted above, if we use a ContextMap the Keys themselves are public API no different than properties on the interface.

[idelpivnitskiy]

I'm for ContextMap. Even though the Key is public is will be 100 times easier to deprecate one key and re-introduce another one if we need any changes to the key type or name vs going through changes on interface.

[idelpivnitskiy]

Also, the key can live inside an internal or a new -experimental package if we really want to experiment without introducing public API

[bryce-anderson]

Putting this discussion on hold for a bit while we actually try out some of these features via an interim API added here.