Expose a constructor for OncRpcClient that takes a OncRpcSvcBuilder

[harrv]

I have a need to supply a threadPoolSize to the OncRpcSvcBuilder's withSelectorThreadPoolSize method for generated clients.

Is there already a way? If not, I'm happy to provide a PR. I see that in the past that's been done by adding additional constructors to OncRpcClient.java. Maybe the most recent time was for PR 53?

Before I give you a PR, I wanted to ask, would it be useful to have a OncRpcClient constructor that takes an instance of OncRpcSvcBuilder so that we wouldn't need additional constructors every time we wanted to pass in a parameter that isn't already exposed? For example, I only need to limit the size of the selectorThreadPool but maybe someone else would need to limit the size of the workerThreadPool next month and would need to go through this process again. If one of the constructors took a OncRpcSvcBuilder then it could also make sure that everything that is needed for a client is set, like .withClientMode() and .withPort(localPort), but otherwise allow the end user to construct the client service with everything else allowed by the builder.

I'm thinking in my PR I'd leave all of the existing constructors alone (so it would be backward compatible with existing code) but change the one that all of the other's call, making it look like this:

public OncRpcClient(InetSocketAddress socketAddress, int protocol, int localPort, IoStrategy ioStrategy, String serviceName) {
    this(socketAddress, localPort, new OncRpcSvcBuilder()
            .withIpProtocolType(protocol)
            .withIoStrategy(ioStrategy)
            .withServiceName(serviceName));
}

and then I'd add this one (called by the one above, but could also be used directly by end users who need it like me):

public OncRpcClient(InetSocketAddress socketAddress, int localPort, OncRpcSvcBuilder builder) {
    _socketAddress = socketAddress;
    _rpcsvc = (builder == null ? new OncRpcSvcBuilder() : builder)
            .withClientMode()
            .withPort(localPort)
            .build();
}

I wanted to vet that idea with you before I do the work of committing a PR. It seems like it would prevent additional future churn just to expose additional things that the builder already provides.

Thanks for your help and input.

[kofemann]

Looks like a reasonable request. However, the OncRpcClient is there just to provide a sensible defaults. If initialization done by the builder, then where is the added value of
OncRpcClient?

  var clnt = new OncRpcSvcBuilder()
          .withoutAutoPublish()
          .withTCP()
          .withClientMode()
          .withWorkerThreadIoStrategy()
          .withSelectorThreadPoolSize(1)
          .withWorkerThreadPoolSize(1)
          .withRpcService(new OncRpcProgram(PROGNUM, PROGVER), upper)
          .withSSLContext(sslClientContext)
          .withServiceName("clnt")
          .build();
  clnt.start();

  RpcTransport t = clnt.connect(svc.getInetSocketAddress(IpProtocolType.TCP));
  clntCall = new RpcCall(PROGNUM, PROGVER, new RpcAuthTypeNone(), t);

The problem with OncRpcSvcBuilder is that it allows to build an invalid service for the client. Maybe a special OncRpcClientBuilder will be a better option, that will take care that internally used OncRpcSvcBuilder built correctly, something like:

  public class OncRpcClient {
     // package only
     OncRpcClient(OncRpcSvc svc) {...}
  }
  
  public class OncRpcClientBuilder {
     
     svcBuilder = new OncRpcSvcBuilder()
           .withClientMode()
           .withoutAutoPublish()
  
     public OncRpcClientBuilder withSelectorThreadPoolSize(int n) {
          svcBuilder.withSelectorThreadPoolSize(1);
          return this;
     }
     
     public OncRpcClient build() {
         return new OncRpcClient(svcBuilder.build());
     }
  }

What do you think?

[harrv]

@kofemann Yes, using a OncRpcClientBuilder does seem better based on the concern you shared. I had attempted to address that in my proposal by setting .withClientMode() on the builder once it was passed to the new constructor, but I'm new to using any of the dCache projects so didn't immediately recognize the other things that are probably server-specific such as auto publish and withRpcService.

If you are willing to make the change that might be better than if I were to do it since you have a good idea of how it needs to be done to make sure users don't end up with an unusable client. If you'd like me to take a stab at it based on your OncRpcClientBuilder idea, let me know and I'll create a PR for mainline following the things you brought up in your post.

Either way though, I'm wondering if you'd be willing to backport it to a version 3.1.2? The project where we're using this is still on Java 8, so we can't go to 3.2 and later quite yet. If that's not possible, I'll probably need to explore taking a fork to 3.1.2 for this.

[kofemann]

I guess, backport to 3.1 is ok, as long as we don't change the behavior. I will try to add a basic implementation that you can enrich if needed.

[harrv]

Thank you!

As long as it gives a way to set both selectorThreadPoolSize and workerThreadPoolSize then I don't think I'll need any other change. The thing that was causing us trouble was when the thread pool size was automatically set based on number of CPU cores.

And just to add, if you decide you'd rather add a constructor that takes those two additional things rather than implement a builder, that would be fine too and is up to you. The idea of using a builder is appealing just in case there is anything else I need to adjust in the future, but I don't currently have a need for anything more than those two additional values.

[kofemann]

could you check that commit kofemann@42fb509 does what you need

[harrv]

That looks great! I think I would also need a corresponding change to jrpcgen.java in the dumpClient() method beginning at about line 1955.

[kofemann]

The rpcgen is updated as well.

[harrv]

Thank you very much. That looks good to me.

[harrv]

@kofemann Can I help with any next steps toward a release (and the 3.1 backport we talked about)?