If we're going to use dependency injection on the Java side, I recommend using Dagger2 since it's much easier to maintain than other Java DI frameworks. I love DI, and I've thought about introducing it in Pulsar already. However, with that said, DI doesn't necessarily remove dependencies; it just makes them easier to work with and change, and it tends to improve code design and maintainability.
> I believe that having the PulsarAdmin handle is very useful and necessary in some circumstances. > Currently the only way is to add configuration parameters to the > Connector and bootstrap the Pulsar Admin from the Connector. > This comes with two big problems: > - security: you must store in the configuration the credentials to access the same cluster (or other clusters) > - maintainability: you have to handle all of the relevant options of the client, and it is hard to be future proof I think Sijie pointed out the right way to handle security with these cases when he said: > We have been standardized on using `authPlugin` and > `authParams` to construct authentication providers. As an example, you can take a look at this Provider: https://github.com/apache/pulsar/blob/66231e313502bfe74fd28976560d4391c5cdefa5/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/KubernetesSecretsTokenAuthProvider.java#L53 Since the providers are loaded at runtime, you can create new ones and make them available to Pulsar by dropping them in the libs directory and changing the provider class in the broker.conf. I think we can use that same mechanism for the auth layer you're talking about. In terms of adding configurations when configuring functions, I started a discussion on how to enable more customization of function parameters, and that led to this message from Jerry about adding an interception mechanism at that level: https://www.mail-archive.com/dev@pulsar.apache.org/msg04869.html If I understand your concern correctly, it looks like that might address some of the functionality you are seeking. I'll be happy to pair on that work once I finish fixing the freezing subscription issue that's been consuming most of my attention (https://github.com/apache/pulsar/issues/6054). -- Devin G. Bost On Fri, May 7, 2021, 6:48 PM Sijie Guo <guosi...@gmail.com> wrote: > Enrico, > > I think there are a couple of different things coupled in this proposal. > Let's make them clear before talking about dependency injection. > > 1) The main problem of PulsarAdmin is not dependency injection. The main > problem is PulsarAdmin uses classes from 3rd party dependencies. > PR#9842 <https://github.com/apache/pulsar/pull/9842> that Rui is working > on > is trying to eliminate the usage of 3rd party classes in its API > signatures. I don't see how dependency injection will address the problem > without PR#9842. > > 2) PulsarClient doesn't have a problem with PulsarAdmin. In fact, we have > used the API from PulsarClient in Context, i.e ConsumerBuilder, > TypeMessageBuilder, Message, and etc. So there is no extra dependency > introduce in PR#8668. So let's not couple #8668 and #10484 with the > discussion of dependency injection. > > 3) I would suggest NOT doing any *dirty hack* was what you were trying to > do in PR#10484. We have been standardized on using `authPlugin` and > `authParams` to construct authentication providers. We shouldn't just add a > special implementation for token authentication in PR#10484. If your team > wants to maintain this *hack*, please maintain it in your own branch not > push it to upstream master. > > Now, we go back to the idea of dependency injection. Although I like the > idea of dependency injection and it makes code clean, I don't support this > change. Because this change introduces an **API** that only works for Java > and doesn't or is hard to support in other languages. We have been > following a principle in designing Functions API to make it as consistent > as possible across different language APIs. I would try to avoid making any > exceptions to this principle if possible. Hope this makes sense to you. > > Thanks, > Sijie > > > > > > On Fri, May 7, 2021 at 6:39 AM Enrico Olivelli <eolive...@gmail.com> > wrote: > > > Hello, > > currently in 2.8.0-SNAPSHOT we added an API to access PulsarAdmin from > > a Pulsar Sink Connector. > > > > The Api is Context.getPulsarAdmin and Context.getPulsarAdmin(clusterName) > > > > > > > https://github.com/apache/pulsar/blob/0469dfe2c7804bd9ca9ea34e95d83b2196216cf9/pulsar-functions/api-java/src/main/java/org/apache/pulsar/functions/api/Context.java#L272 > > > > With this API we introduced a hard link between the Client Admin API > > and the Functions API. > > > > If I understand correctly this is undesirable as this implies that the > > Functions API now depends on the Pulsar Admin API. > > > > I believe that having the PulsarAdmin handle is very useful and > > necessary in some circumstances. > > Currently the only way is to add configuration parameters to the > > Connector and bootstrap the Pulsar Admin from the Connector. > > This comes with two big problems: > > - security: you must store in the configuration the credentials to > > access the same cluster (or other clusters) > > - maintainability: you have to handle all of the relevant options of > > the client, and it is hard to be future proof > > > > The same happens when you need a Pulsar Client. > > > > I have a proposal to resolve this problem, before we cut 2.8.0 and we > > make it a stable API. > > > > What about introducing a little bit of "dependency" injection in the > > Pulsar Functions framework ? > > > > Legacy code: > > > > class MyClass { > > public X myCode(Y) { > > PulsarAdmin admin = context.getPulsarAdmin(); > > admin.doSomething(); > > } > > } > > > > Dependency injection style code: > > > > class MyClass { > > @Resource > > PulsarAdmin admin; > > > > @Resource > > PulsarClient pulsarClient; > > > > public X myCode(Y) { > > admin.doSomething(); > > } > > > > } > > > > this way we do not have a hard reference from "Context" to PulsarAdmin > > and to PulsarClient, the dependency can be resolved at runtime. > > > > In order to implement getPulsarAdmin(clusterName) we can introduce a > > new API PulsarAdminLocator in the Pulsar Admin API package. > > > > interface PulsarAdminLocator { > > PulsarAdmin getPulsarAdmin(String cluster); > > } > > > > class MyClass { > > @Resource > > PulsarAdminLocator adminLocator; > > > > public X myCode(Y) { > > PulsarAdmin admin = adminLocator.getPulsarAdmin(cluster); > > admin.doSomething(); > > } > > > > } > > > > Does it make sense ? > > > > I will be happy to follow up with a PIP > > > > This work is particularly important in order to fix many issues we > > have on Connectors, when you need to access Pulsar itself. > > > > for reference: > > https://github.com/apache/pulsar/pull/8668 [pulsar-io] Support > > authentication on debezium connector > > https://github.com/apache/pulsar/pull/10484 Support Pulsar Auth token > > for Debezium and Kafka connect adaptor > > > > > > > > Enrico > > >
