[opamp-client] Refactor opamp client callbacks api

[breedx-splk]

Resolves #2250.

This is one approach that I thought was a reasonable compromise. The client implementation has a mutable/volatile Callbacks instance which is set in the create() factory method. Rather than passing a Callbacks instantly, the user passes a Function that accepts the client instance a returns a Callbacks instance.

In practice, I suspect that most users won't be creating anonymous Callback instances like in the README.md example, and that instead they will pass the client to the constructor of their concrete implementations.

[breedx-splk]

I don't love the mutable/volatile Callbacks instance, but it shouldn't be high-volume usage and isn't mutable from outside the impl.

[LikeTheSalad]

Thank you for taking a look into this issue, @breedx-splk. At first glance, it's an improvement from the previous approach, although it feels to me that it mostly moves the issue further away, but it's not fully fixing it. That might be enough, though, so I'm not closed to the idea of moving forward with this approach, but before that, I'd like to share some concerns with you and see if those make sense to you.

Regarding:

In practice, I suspect that most users won't be creating anonymous Callback instances like in the README.md example, and that instead they will pass the client to the constructor of their concrete implementations.

I agree that using concrete implementations should be the most common use case, though I think that, for this approach of passing a function, it mostly makes sense if the user is fine with creating their implementation inside, like so:

public static void main(String[] args) {
  OpampClient client = OpampClient.builder().build(c -> new MyCallbacks(c));
}

However, if for some reason a user would like to have their callbacks stored in a variable, it becomes awkward to achieve so, as the 🐔 🥚 problem remains:

public static void main(String[] args) {
  MyCallbacks myCallbacks = new MyCallbacks(client); // compilation error.
  OpampClient client = OpampClient.builder().build(c -> myCallbacks);
}

There are ways to work around it, such as using an atomic reference, but it seems cumbersome to me. This might be a use case that could be present in factory methods, for example, during the creation of a "manager" for opamp that has fields for both the client object and the callbacks, or that maybe the manager itself implements the callbacks. In those cases, I think the api still feels cumbersome, even with the function param.

And regarding:

This is one approach that I thought was a reasonable compromise.

I noticed that these changes are quite subtle in that they don't change much of the API, and my understanding of this statement is that it is by design, to avoid affecting existing users too much. If that's the case, I appreciate the intention, but considering that this is marked as an incubating project and that it was recently released, I think this is the right time to make "groundbreaking changes" if needed.

With that in mind, I'm curious to learn your opinion on the use case I mentioned above, and considering that it shouldn't be needed to try and make compromises at this stage of the project, how do you see this approach compare with the alternatives mentioned here in terms of ease of use for different use cases?

[breedx-splk]

Some valid points, @LikeTheSalad ...and I think you squarely understand the tradeoffs. I'll respond here.

As far as declaring the callbacks in a variable, it's possible, as you noted, clunky.

public static void main(String[] args) {
  MyCallbacks myCallbacks = new MyCallbacks(); // NOT a compilation error.
  OpampClient client = OpampClient.builder().build(c -> {
    myCallbacks.setClient(c); // requires callbacks to be mutable if they care about the client
    return myCallbacks;
  });
}

...or the atomic reference approach you suggested, but we won't show that here.

I do give preference for smaller changes, but I also agree that now is the time to do big/breaking changes as much as we like. I really just wanted to propose an implementation that had different (and in my view, very slightly better) trade offs than the other two approaches.

If you are hesitant for this, that's fine and I understand. If we want to go with the other alternatives suggested in #2250, I think I would choose to add the client as a parameter in the callback methods...but it sounded like you didn't really like that idea. Maybe I'm misunderstanding...but that solves the problem pretty cleanly as well without making the API unwieldy.

If you wanna go that route instead, I'm happy to close this.

[LikeTheSalad]

Thank you for the detailed response and for understanding, @breedx-splk.

If we want to go with the other alternatives suggested in #2250, I think I would choose to add the client as a parameter in the callback methods...but it sounded like you didn't really like that idea. Maybe I'm misunderstanding...but that solves the problem pretty cleanly as well without making the API unwieldy.

It's not my favorite one, although I can't find an easy way to mess things up with it if I were a user. That's usually how I pick between API approaches, by trying to think of ways to easily misuse them or to get into roadblocks that would require "creative solutions" on behalf of the users. Though I can always miss things, as it happened with the existing API when I was asked to remove its start/stop methods. It's nicer to have just a close() method from our perspective tbh, as we don't need to validate things such as avoiding start() to be called more than once, and also it isn't needed to have a nullable field for the callback, but I didn't think of what it could mean from a practical point of view when needed to interact with the client from within the callback, which should be quite common. That's why I added an open question to the "client in the callback methods" option on my comment, because I can't find something particularly bad with it, but was wondering if someone else was aware of something.

So far, I've only gotten feedback on it from @trask, which may or may not have been due to some potential issues that he might be aware of, so I'll loop him in just in case. Though if there's no issue, I agree that the "client in the callback methods" is an option that doesn't break too much of the existing API and fully fixes the problem, so I'd be more inclined to go with that approach instead of the one from this PR.

[trask]

So far, I've only gotten feedback on it from @trask, which may or may not have been due to some potential issues that he might be aware of, so I'll loop him in just in case.

I'm good with whatever y'all decide

[breedx-splk]

Ok, sounds like @LikeTheSalad is slightly in favor of another approach (which provides the client in the callback methods), so let's close this and see about that other way. 🥂