Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement events #440

Closed
Tracked by #437
toddbaert opened this issue May 19, 2023 · 12 comments · Fixed by #476
Closed
Tracked by #437

Implement events #440

toddbaert opened this issue May 19, 2023 · 12 comments · Fixed by #476
Assignees
Labels

Comments

@toddbaert
Copy link
Member

toddbaert commented May 19, 2023

I'd like to implement events as mentioned here.

At the moment, I'm trying to decide on the most "Java-ish" way of implementing an event listener. In the web-sdk, we add event handlers like this:

client.addHandler(ProviderEvents.Ready, myClientOnReadyHandler);

Something very like this is possible in Java, but it doesn't feel very "Java-y". I think the spec gives us the flexibility to do something more like this:

client.addEventObserver(new EventObserver() { // anonymous class implementing possible EventObserver interface

  @Override
  public onProviderReady(EventDetails eventDetails) {
    // do stuff
  }
  
  @Override
  public onProviderConfigurationChanged(EventDetails eventDetails) {
    // do stuff
  }
  
  @Override
  public onProviderError(EventDetails eventDetails) {
    // do stuff
  }

});

What are people's thoughts?

cc @open-feature/sdk-java-approvers @open-feature/sdk-java-maintainers

@kinyoklion
Copy link
Member

I am not super engaged in the Java ecosystem, but how do people feel about SAM interfaces these days? For me it seems like the ability to provide a lambda when I just care about one aspect is nice, especially when they seem like reasonably different concerns. Though I imagine that would mean 3 registration methods.

@toddbaert
Copy link
Member Author

I am not super engaged in the Java ecosystem, but how do people feel about SAM interfaces these days? For me it seems like the ability to provide a lambda when I just care about one aspect is nice, especially when they seem like reasonably different concerns. Though I imagine that would mean 3 registration methods.

I like functional interfaces, and I think that's basically how I would tackle the first example above... I fear I might be in the minority there though 😅 . I'm hoping that a discussion here can help us decide.

gRPC decided to go with something like my second proposal, FWIW: https://grpc.io/docs/languages/java/basics/#bidirectional-streaming-rpc https://grpc.github.io/grpc-java/javadoc/io/grpc/stub/StreamObserver.html

@justinabrahms
Copy link
Member

I'll defer to others. I don't have experience with eventing within Java like this. I would naturally reach for unlined lambdas as others mention.

@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented May 19, 2023

I am also in favor of lambda, even if that requires individual registrations. This gives the developer the freedom to register only on interested events.

Besides, I think we will only need a single functional interface definition from SDK 🤔

@FunctionalInterface
public interface EventHandler {
    void handle(EventDetails details);
}

p.s - I think we can't fully compare the grpc stream observer to OF events. The grpc interface is a contract for the full request streaming lifecycle (new response, error and done), where as OF is purely individual events.

@toddbaert
Copy link
Member Author

OK I think I'll go with the functional interface direction then!

@lopitz
Copy link
Contributor

lopitz commented May 20, 2023

👍 to the functional interface direction.

I happen to love the project reactor api (e.g. Mono) over the last couple of years and I think, it would be a wonderful fit for this implementation as well. The signature of the event listener registration methods would be something like

Client doOnProviderReady(Consumer<EventDetails> eventHandler)

or we could also (additionally?) go the route

Client doOnProviderEvent(Predicate<ProviderEvent> predicate, Consumer<EventDetails> eventHandler)

do allow for new event types.

A code example could be:

client
    .doOnProviderReady(eventDetails -> doSomethingWhenTheProviderIsReady(eventDetails))
    .doOnProviderError(this::handleProviderError) //alternative notation
    .doOnProviderEvent(eventType -> eventType == ProviderEvents.ConfigurationChanged, this::providerConfigurationChanged);

To make the handling of the predicates easier, we could even move the predicates into the ProviderEvents, so that it might read like

client
    .doOnProviderEvent(ProviderEvents::isConfigurationChanged, this::providerConfigurationChanged);

@lopitz
Copy link
Contributor

lopitz commented May 20, 2023

I am also in favor of lambda, even if that requires individual registrations. This gives the developer the freedom to register only on interested events.

Besides, I think we will only need a single functional interface definition from SDK 🤔

@FunctionalInterface
public interface EventHandler {
    void handle(EventDetails details);
}

p.s - I think we can't fully compare the grpc stream observer to OF events. The grpc interface is a contract for the full request streaming lifecycle (new response, error and done), where as OF is purely individual events.

I'm not sure, whether we need a dedicated interface definition at all. The JDK already contains a Consumer<T> interface, which should be sufficient for this job.

@Kavindu-Dodan
Copy link
Contributor

I am also in favor of lambda, even if that requires individual registrations. This gives the developer the freedom to register only on interested events.
Besides, I think we will only need a single functional interface definition from SDK 🤔

@FunctionalInterface
public interface EventHandler {
    void handle(EventDetails details);
}

p.s - I think we can't fully compare the grpc stream observer to OF events. The grpc interface is a contract for the full request streaming lifecycle (new response, error and done), where as OF is purely individual events.

I'm not sure, whether we need a dedicated interface definition at all. The JDK already contains a Consumer<T> interface, which should be sufficient for this job.

Yes, agree with this. I just showed the possibility of minimalizing the contract.

@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented May 22, 2023

I happen to love the project reactor api (e.g. Mono) over the last couple of years and I think, it would be a wonderful fit for this implementation as well. The signature of the event listener registration methods would be something like

@lopitz I haven't used Mono personally but checked the referenced docs. It is interesting but I have doubts.

API does provide many features, but do you see how this fits especially to a library scenario? The API we have accepts an event handler and the internals of the handlers are beyond our scope (ex:- accepting an event through a simple logger, running a background thread, etc...). Whereas Mono focuses on reacting to data streams (per my understanding and in general) with extra operations (ex:- delay, take(x))

@lopitz
Copy link
Contributor

lopitz commented May 23, 2023

@Kavindu-Dodan i just meant, that the Mono API reads well. We of course don't need all the stuff that is part of the Mono API. so, it's really just the naming of methods we need, not trying to pull in methods, which are available. Hence, with the current spec I see 4 methods, which would be beneficial. They would be:

public Client doOnProviderReady(Consumer<EventDetails> eventConsumer);
public Client doOnProviderError(Consumer<EventDetails> eventConsumer);
public Client doOnProviderConfigurationChanged(Consumer<EventDetails> eventConsumer);

//the following method could also be omitted
public Client doOnProviderEvent(Predicated<ProviderEventType> condition, Consumer<EventDetails> eventConsumer);

@Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan i just meant, that the Mono API reads well. We of course don't need all the stuff that is part of the Mono API. so, it's really just the naming of methods we need, not trying to pull in methods, which are available. Hence, with the current spec I see 4 methods, which would be beneficial. They would be:

public Client doOnProviderReady(Consumer<EventDetails> eventConsumer);
public Client doOnProviderError(Consumer<EventDetails> eventConsumer);
public Client doOnProviderConfigurationChanged(Consumer<EventDetails> eventConsumer);

//the following method could also be omitted
public Client doOnProviderEvent(Predicated<ProviderEventType> condition, Consumer<EventDetails> eventConsumer);

Thanks for the explanantion @lopitz , Yes, agree with the naming and parameters. Plus the chaining capability by returning the Client

@toddbaert
Copy link
Member Author

toddbaert commented Jun 1, 2023

I've started on this, based on #456, with an APi similar to what @lopitz has proposed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants