Initial client review

Author: philwebb

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/ChannelBuilderCustomizers.java

@@ -34,6 +34,8 @@
  */
 public class ChannelBuilderCustomizers {
 
+	// FIXME PW: Make package private?
+
 	private final List<GrpcChannelBuilderCustomizer<?>> customizers;
 
 	ChannelBuilderCustomizers(List<? extends GrpcChannelBuilderCustomizer<?>> customizers) {

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/ClientInterceptorsConfiguration.java

@@ -31,6 +31,8 @@
 @Configuration(proxyBeanMethods = false)
 public class ClientInterceptorsConfiguration {
 
+	// FIXME PW: Make package private?
+
 	@Bean
 	@ConditionalOnMissingBean
 	ClientInterceptorsConfigurer clientInterceptorsConfigurer(ApplicationContext applicationContext) {

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/ClientScanConfiguration.java

@@ -44,6 +44,10 @@
 @Import(DefaultGrpcClientRegistrations.class)
 public class ClientScanConfiguration {
 
+	// FIXME PW: Package private?
+
+	// FIXME PW: Why inner class? AbstractGrpcClientRegistrar protection perhaps?
+	// FIXME PW: Swith from Import to Bean method?
 	static class DefaultGrpcClientRegistrations extends AbstractGrpcClientRegistrar
 			implements EnvironmentAware, BeanFactoryAware {
 
@@ -66,6 +70,7 @@ protected GrpcClientRegistrationSpec[] collect(AnnotationMetadata meta) {
 			Assert.notNull(this.environment, "Environment must not be null");
 			Assert.notNull(this.beanFactory, "BeanFactory must not be null");
 			Binder binder = Binder.get(this.environment);
+			// FIXME PW: Why direct binding and not properties injection?
 			boolean hasDefaultChannel = binder.bind("spring.grpc.client.default-channel", ChannelConfig.class)
 				.isBound();
 			if (hasDefaultChannel) {

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/ConditionalOnGrpcClientEnabled.java

@@ -42,4 +42,6 @@
 @ConditionalOnProperty(prefix = "spring.grpc.client", name = "enabled", matchIfMissing = true)
 public @interface ConditionalOnGrpcClientEnabled {
 
+	// FIXME PW: package private?
+
 }

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/GrpcChannelFactoryConfigurations.java

@@ -62,6 +62,7 @@ ShadedNettyGrpcChannelFactory shadedNettyGrpcChannelFactory(GrpcClientProperties
 				ChannelCredentialsProvider credentials) {
 			List<GrpcChannelBuilderCustomizer<io.grpc.netty.shaded.io.grpc.netty.NettyChannelBuilder>> builderCustomizers = List
 				.of(channelBuilderCustomizers::customize);
+			// FIXME PW: sweep for
 			var factory = new ShadedNettyGrpcChannelFactory(builderCustomizers, interceptorsConfigurer);
 			factory.setCredentialsProvider(credentials);
 			factory.setVirtualTargets(properties);

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/GrpcChannelFactoryCustomizer.java

@@ -20,6 +20,9 @@
 
 public interface GrpcChannelFactoryCustomizer {
 
+	// FIXME PW: Doc comment
+	// FIXME PW: Function interface?
+
 	/**
 	 * Customize the given {@link GrpcChannelFactory}.
 	 * @param factory the factory to customize

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/GrpcClientObservationAutoConfiguration.java

@@ -36,6 +36,8 @@
 
 public final class GrpcClientObservationAutoConfiguration {
 
+	// FIXME PW: Should grpc know about observation or should we flip it?
+
 	@Bean
 	@GlobalClientInterceptor
 	@ConditionalOnMissingBean

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/GrpcClientProperties.java

@@ -39,6 +39,9 @@
 @ConfigurationProperties(prefix = "spring.grpc.client")
 public class GrpcClientProperties implements EnvironmentAware, VirtualTargets {
 
+	// FIXME PW: Properties should be anemic. No implemented EnvironmentAware or
+	// VirtualTargets
+
 	/**
 	 * The default channel configuration to use for new channels.
 	 */
@@ -54,6 +57,8 @@ public class GrpcClientProperties implements EnvironmentAware, VirtualTargets {
 	 */
 	private Class<? extends StubFactory<?>> defaultStubFactory = BlockingStubFactory.class;
 
+	// FIXME PW: Unusual to have class names in properties. Should we do this another way?
+
 	private Environment environment;
 
 	GrpcClientProperties() {
@@ -90,13 +95,16 @@ public void setEnvironment(Environment environment) {
 	 * default channel as a template
 	 */
 	public ChannelConfig getChannel(String name) {
+		// FIXME PW: Unusual logic for properties class
+		// FIXME PW: Check callers of this method. What's name mean?
 		if ("default".equals(name)) {
 			return this.defaultChannel;
 		}
 		ChannelConfig channel = this.channels.get(name);
 		if (channel != null) {
 			return channel;
 		}
+		// FIXME PW: Looks like convention. Move somewhere else?
 		channel = this.defaultChannel.copy();
 		String address = name;
 		if (!name.contains(":/") && !name.startsWith("unix:")) {
@@ -134,6 +142,8 @@ public static class ChannelConfig {
 		 */
 		private String address = "static://localhost:9090";
 
+		// FIXME PW: All privates at top then getter / setters
+
 		public String getAddress() {
 			return this.address;
 		}
@@ -351,6 +361,7 @@ public void setDefaultDeadline(@Nullable Duration defaultDeadline) {
 		 * @return a copy of the channel instance.
 		 */
 		public ChannelConfig copy() {
+			// FIXME PW: What's this for? Pretty unusual in a properties class
 			ChannelConfig copy = new ChannelConfig();
 			copy.address = this.address;
 			copy.defaultLoadBalancingPolicy = this.defaultLoadBalancingPolicy;
@@ -407,6 +418,7 @@ public static class Ssl {
 			 */
 			private @Nullable String bundle;
 
+			// FIXME PW: Anemic getters / getter
 			public boolean isEnabled() {
 				return (this.enabled != null) ? this.enabled : this.bundle != null;
 			}
@@ -434,6 +446,8 @@ public void copyValuesFrom(Ssl other) {
 
 		}
 
+		// FIXME PW: Review how heath works.
+
 		public static class Health {
 
 			/**

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/NamedChannelCredentialsProvider.java

@@ -38,6 +38,8 @@
  */
 public class NamedChannelCredentialsProvider implements ChannelCredentialsProvider {
 
+	// FIXME PW: Package private?
+
 	private final SslBundles bundles;
 
 	private final GrpcClientProperties properties;

module/spring-boot-grpc-client/src/main/java/org/springframework/boot/grpc/client/autoconfigure/codec/GrpcCodecConfiguration.java

@@ -41,6 +41,8 @@ public class GrpcCodecConfiguration {
 	@Bean
 	@ConditionalOnMissingBean
 	CompressorRegistry compressorRegistry(ObjectProvider<Compressor> compressors) {
+		// FIXME PW: @ExperimentalApi ??
+		// FIXME PW: getDefaultInstance() is a static. Is that what we want?
 		CompressorRegistry registry = CompressorRegistry.getDefaultInstance();
 		compressors.orderedStream().forEachOrdered(registry::register);
 		return registry;