Virtual targets and duplicate entries in channels map

[therepanic]

Refers to #67

[dsyer]

Looks like we need a rebase? Also maybe NamedChannelCredentialsProvider can move into core as well?

[therepanic]

Looks like we need a rebase? Also maybe NamedChannelCredentialsProvider can move into core as well?

I'm not quite sure, but I don't think so. We can't use from core, for example SslBundles or GrpcClientProperties.

Also an interesting point. Right now the build is crashing for obvious reasons. If we look carefully in the log, we can see that:

[ERROR] GrpcClientPropertiesTests$BindPropertiesAPI.withoutKeepAliveUnitsSpecified:125 
expected: 1S
 but was: 0.001S

This is due to the fact that from core @DurationUnit (which I removed from NamedChannel as it is not reachable) was responsible as I understand for converting from application.properties to a specific value (seconds) if no unit is specified. But now the default unit of measure is milliseconds.

I guess we should think about how to play this properly to handle time correctly?

[onobc]

Looks like we need a rebase? Also maybe NamedChannelCredentialsProvider can move into core as well?

I'm not quite sure, but I don't think so. We can't use from core, for example SslBundles or GrpcClientProperties.

Also an interesting point. Right now the build is crashing for obvious reasons. If we look carefully in the log, we can see that:

[ERROR] GrpcClientPropertiesTests$BindPropertiesAPI.withoutKeepAliveUnitsSpecified:125 
expected: 1S
 but was: 0.001S

This is due to the fact that from core @DurationUnit (which I removed from NamedChannel as it is not reachable) was responsible as I understand for converting from application.properties to a specific value (seconds) if no unit is specified. But now the default unit of measure is milliseconds.

I guess we should think about how to play this properly to handle time correctly?

I'm going to pull down and take a look closer. Ulitmately we will want the auto-configuration to map from client properties to the created named channels. The current named channel was doing both setting of defaults as well as representing channels. More likely I would expect a NamedChannel record that is constructed from the NamedChannel properties.

[therepanic]

I guess we should still move the NamedChannel migration to another PR then, and put the test writing and fixing into this one (or should we?).

By the way, I don't know the project very well and I would like you to help me a bit. Am I right that this code can help us and be a test?

ClientInterceptorsConfigurer configurer = mock();

DefaultGrpcChannelFactory factory = new DefaultGrpcChannelFactory(List.of(), configurer);

ManagedChannel logicalChannel = factory.createChannel("logicalName").build();
ManagedChannel physicalChannel = factory.createChannel("static://localhost:9090").build();

assertThat(logicalChannel).isSameAs(physicalChannel);

[onobc]

I guess we should still move the NamedChannel migration to another PR then,

I think that is a good approach.

and put the test writing and fixing into this one (or should we?).

I think so.

By the way, I don't know the project very well and I would like you to help me a bit. Am I right that this code can help us and be a test?

ClientInterceptorsConfigurer configurer = mock();

DefaultGrpcChannelFactory factory = new DefaultGrpcChannelFactory(List.of(), configurer);

ManagedChannel logicalChannel = factory.createChannel("logicalName").build();
ManagedChannel physicalChannel = factory.createChannel("static://localhost:9090").build();

assertThat(logicalChannel).isSameAs(physicalChannel);

I will take a look at the above code sometime in the next few hours.

[onobc]

Hi @panic08 ,

The particular "duplicate entries" can be seen when there is a named channel configured w/ an address as follows:

spring.grpc.client.channels.custom.address=static://0.0.0.0:0

Then a call to DefaultGrpcChannelFactory.createChannel("custom") results in an extra entry in the NamedVirtualTargets channels map.

Let's look at the call chain to understand what's going on...

DefaultGrpcChannelFactory.createChannel("custom")
	NamedChannelVirtualTargets.getTarget("custom")
		GrpcClientProperties.getChannel("custom")
		GrpcClientProperties.getTarget("static://0.0.0.0:0")
			GrpcClientProperties.getChannel("static://0.0.0.0:0")
			GrpcClientProperties.getTarget("static://0.0.0.0:0")

On line 4 above, you can see getTarget called w/ the actual address which results in the GrpcClientProperties.getTarget doing a lookup in the channels map (computeIfAbsent).

The main issue is that NamedChannelVirtualTargets.getTarget calls GrpcClientProperties.getTarget with the channel address which in turn goes back through the getChannel+getTarget with the address.

You can add the following test to org.springframework.grpc.autoconfigure.client.GrpcClientPropertiesTests.GetTargetAPI

@Test
void withCustomChannelReturnsDuplicateEntryMap() {
	Map<String, String> map = new HashMap<>();
	map.put("spring.grpc.client.channels.custom.address", "static://my-server:8888");
	GrpcClientProperties properties = bindProperties(map);
	NamedChannelVirtualTargets virtualTargets = new NamedChannelVirtualTargets(properties);
	var address = virtualTargets.getTarget("custom");
	assertThat(address).isEqualTo("my-server:8888");
	assertThat(properties.getTarget("custom")).isEqualTo("my-server:8888");
	assertThat(properties.getChannels()).containsOnlyKeys("default", "custom");
}

I know it is a lot of details but just let me know if you need more clarification etc..

[therepanic]

Thank you very much for your clarification

In NamedChannelVirtualTargets.getTarget I have removed the double call and replaced with

@Override
public String getTarget(String authority) {
	return this.channels.getChannel(authority).getAddress();
}

and also added the test in GrpcClientPropertiesTests.GetTargetAPI you helped to write.

Also would like to understand. Am I correct in understanding that you made a slight mistake and instead of
assertThat(properties.getTarget(“custom”)).isEqualTo(“my-server:8888”);
should be
assertThat(properties.getTarget(“custom”)).isEqualTo(“static://my-server:8888”);

[therepanic]

Hmm. As I understand it, we can use GrpcClientProperties.getChannel to get the address. We also want to commit getTarget for the address, but without calling getChannel. I will think about how this can best be done.

[onobc]

Also would like to understand. Am I correct in understanding that you made a slight mistake and instead of

No it is intentional, as the GrpcClientProperties.getTarget strips the static prefix off:

if (address.startsWith("static:") || address.startsWith("tcp:")) {
	address = address.substring(address.indexOf(":") + 1).replaceFirst("/*", "");
}

[therepanic]

I apologize for the inactivity of two weeks.

To solve this problem, I thought about it and decided to resort to overloading the getTarget method in GrpcClientProperties with the NamedChannel argument.

I also decided that it would make sense to change
assertThat(properties.getChannels()).containsOnlyKeys(“default”, “custom”);

which you recommended to
assertThat(properties.getChannels()).containsOnlyKeys(“custom”);

I did it because of the acbe334

[onobc]

Thanks for the contribution @panic08 - nice work.