Expose a constructor for OncRpcClient that takes a OncRpcSvcBuilder about oncrpc4j

[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.

[kofemann]

The rpcgen is updated as well.

[kofemann]

I will try to publish the jars over the weekend

[kofemann]

The jars should be online.

[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.

[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.

[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)?

[harrv]

The jars should be online.

Thanks so much for all of your work on that! Using version 3.1.2 now in our project.