refactor: remove channel monitor

[toddbaert]

This PR removes the channel monitor (see thread here).. as Guido put it:

The current implementation only emits errors messages on transient connection errors or shutdown. Whatever the ChannelConnector's responsibility was, it seems to be handled by other components by now.

Reasons/justification:

• any errors seen also fire the error handler in the stream logic (this is why our e2e tests pass without this)
• it relied on an experimental gRPC API
• it potentially caused duplicate error events (one from the monitor, one from the stream)

I believe this was previously needed before we made the eventStream (RPC mode) required, and before @guidobrei optimized and commonized our streaming logic. Now, I think this code is basically dead.

If this code really is needed for some reason, I think we probably need e2e tests to exercise whatever situation that is, if those can be reasonably created.

I would like to do more refactors around this (for example we can probably delete the entire ChannelConnector class now, but I wanted to try to keep this PR as clear as possible.

[gemini-code-assist]

Summary of Changes

Hello @toddbaert, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the ChannelConnector by completely removing the channel monitor component. This monitor was previously responsible for observing gRPC channel connectivity states and emitting related events. The change is driven by the fact that current stream logic already robustly handles errors, the monitor utilized an experimental gRPC API, and it could lead to redundant error notifications. By removing this component, the connection management becomes simpler, more efficient, and aligns better with the current architecture where eventStream is a mandatory component.

Highlights

• Removal of Channel Monitor: The dedicated channel monitor functionality has been entirely removed from the ChannelConnector class, simplifying its role to solely manage the gRPC channel and its shutdown.
• Streamlined Error Handling: Error detection and handling are now exclusively managed by the existing stream logic, eliminating redundancy and potential duplicate error events previously caused by the channel monitor.
• Dependency on Experimental API Eliminated: The removal addresses the reliance on an experimental gRPC API, contributing to a more stable and maintainable codebase.
• Code Cleanup and Optimization: This change removes what is considered dead code, as the eventStream (RPC mode) is now required and streaming logic has been optimized, rendering the channel monitor obsolete.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively removes the channel monitoring logic, which simplifies the ChannelConnector and removes reliance on an experimental gRPC API. The changes are well-justified and correctly applied across the codebase, including the removal of corresponding tests.

I have a couple of suggestions for further cleanup to complete the refactoring:

• The ChannelConnector constructor signature can be simplified by removing a now-unused parameter.
• The ChannelConnectorTest class is left without any tests and should either be removed or have a test added for the remaining functionality.

I am having trouble creating individual review comments. Click here to see my feedback.

providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/common/ChannelConnector.java (35)

The onConnectionEvent parameter is no longer used within the constructor or the class after this refactoring. It should be removed from the method signature to make the API cleaner and prevent confusion.

This change will require updating the constructor calls in RpcResolver and SyncStreamQueueSource.

            final FlagdOptions options, ManagedChannel channel) {

providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/common/ChannelConnectorTest.java (88-150)

After removing the tests for the channel monitor, ChannelConnectorTest is left with no actual tests. An empty test class can be misleading and adds to maintenance overhead.

Please consider one of the following options:

1. Remove the ChannelConnectorTest.java file completely.
2. Add a new test for the remaining shutdown() functionality to ensure it behaves as expected.

Here is an example test for the shutdown() method:

@Test
void shutdownShouldShutdownChannel() throws InterruptedException {
    ManagedChannel channel = mock(ManagedChannel.class);
    when(channel.isShutdown()).thenReturn(false);

    // The second argument to the constructor is now unused.
    ChannelConnector connector = new ChannelConnector(FlagdOptions.builder().build(), null, channel);
    
    connector.shutdown();
    
    verify(channel).shutdownNow();
    verify(channel).awaitTermination(anyLong(), any(TimeUnit.class));
}

[toddbaert]

Gemini suggested I remove this whole test file. I agree, and I think I should also delete the entire ChannelConnector class as I said in my description, but I think I want to do that separately, as that would require small refactors around shutdown which I don't want to add to this PR.

[guidobrei]

I like all the red removed lines in the diff ❤️

Maybe we should rename the class now to ChannelDisconnector 😆

[toddbaert]

I like all the red removed lines in the diff ❤️

Maybe we should rename the class now to ChannelDisconnector 😆

Haha, ya, I think it's awkward now that it just disconnects, but I want to do that separately. I wanted to make it very clear in this PR what was being removed.