This might not needed when we use getAndCheckFactoryByName appropriately.

I don't think the cluster manager should be responsible for picking the LB policy out of load_balancing_policies. IMO it's better if the cluster info does this instead - then it knows which policy was picked, and doesn't have to carry around config for any policies that weren't picked.

Why we want to do a) check whether the lb policy factory registered , and b) create lb policy from factory in two different places? That doesn't make sense either.

You can pick LB policy in ClusterManagerImpl and store a reference to the lb policy factory in a member in ClusterInfoImpl, and just use that in cluster manager.

I think I'm missing an important detail somewhere... I'm under the impression that we can't store the factory we've picked from the ClusterManagerImpl, since info() returns a const pointer. By the time the cluster manager picks the LB policy it's too late to write what we picked into the ClusterInfoImpl. That's why I was hoping to pick the policy in the ClusterInfoImpl constructor instead.

This is mostly a result of wanting to be able to write a unit test that verifies that the set of policies { 1. policy with no registered factory, 2. policy with a registered factory } will result in the known policy being picked. It seems to me this requires that the ClusterInfoImpl to know which policy was picked, and hence to do the picking itself. I suppose this problem disappears if we don't bother with that bit of the test but it seems to me like a reasonably important behaviour to ensure is correct here.

Arguably a) is not so much about checking whether the factory is registered as much as it is about actually picking the policy - which happens to guarantee as a postcondition that the chosen factory is registered by definition. So I'm not how problematic it is to have policy picking in the ClusterInfoImpl constructor and the actual LB instantiation in the cluster manager instead. What do you think?

I think I'm missing an important detail somewhere... I'm under the impression that we can't store the factory we've picked from the ClusterManagerImpl, since info() returns a const pointer. By the time the cluster manager picks the LB policy it's too late to write what we picked into the ClusterInfoImpl. That's why I was hoping to pick the policy in the ClusterInfoImpl constructor instead.

I meant you can implement lb_policy_factory() in ClusterInfoImpl to return the reference to the factory. My point here is to reduce manual check config and throw EnvoyException to make the error message more consistent and easy to evolve extension API in general, that's why I pointed to just use TypedExtensionConfig in proto part as well.

Done.

Is this referred anywhere? If no just delete?

I was having trouble getting the presubmits to pass when I just deleted, but I didn't realize I needed to deprecate the unused v2 message as well. Done (for the parts of Policy that are removed).

I think I'm missing an important detail somewhere... I'm under the impression that we can't store the factory we've picked from the ClusterManagerImpl, since info() returns a const pointer. By the time the cluster manager picks the LB policy it's too late to write what we picked into the ClusterInfoImpl. That's why I was hoping to pick the policy in the ClusterInfoImpl constructor instead.

I meant you can implement lb_policy_factory() in ClusterInfoImpl to return the reference to the factory. My point here is to reduce manual check config and throw EnvoyException to make the error message more consistent and easy to evolve extension API in general, that's why I pointed to just use TypedExtensionConfig in proto part as well.

We no longer use name in extension config to identify which factory should be used, it is determined by the type_url in typed_config, so this you should use Config::Utility::getAndCheckFactory() and pass the whole message instead.

Done, thanks.

Instead of deprecating this whole message, let's deprecate the name and typed_config fields and add the new TypedExtensionConfig field inside of this message. That way, if we need to add a new field on a per-policy basis in the future, we'll have somewhere to put it.

That seems to be an anti-pattern, what field do we expect? In general a per-policy field should be added inside the message packed into Any.

If it's a field only for one individual policy, that would make sense. But if we discover the need to add another field that we want to be set for all policies, then this wrapper would provide a place to put that.

Could we put that into CommonLbConfig then, and include the message in the policy's message? Or is the idea that policies would need to set this field whether they use the common LB config or not?

The verbosity of load_balancing_policy.policy.typed_extension_config.typed_config.foo seems a bit excessive. Is it something we could change if/when we actually have such a common field?

Not all policies will include CommonLbConfig in their config, and the idea here is to have a way to add the field for any policy -- e.g., if we wanted to add something to express the control flow of how the client picks which policy to use.

We could make this change later, but it would be a breaking change at that point. The idea here is to future-proof a bit.

Ack, done.

I don't think this needs to be deprecated in v2. It doesn't really make sense to do this unless we're also adding the new field to v2, and I don't see any need for that.

Instead, these fields should be kept in v3 and marked as deprecated there.

Done.

Thanks! The API changes look great here, but I'll let Lizan approve.

I thought we can delete them instead of deprecating since the whole message was not-implemented-hide before, and the implementation doesn't support this either.

I'd be okay with fully removing these fields too, unless that causes problems. But I think there's very little cost to leaving them here and marking them deprecated.

Hmm. I tried to remove the whole Policy message from v3 only earlier and it failed the presubmit because it was expecting some v3 message to correspond to the v2 version of the same message. (That's why I ended up deprecating v2 fields in an earlier commit, which I've since undone.)

Removing these fields seems to be fine, though, so I've done that.

Looks good to me!

Thanks. While there's little cost to leaving them here, but then the doc will be confusing as we never use those field in implementation.