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

feat: better approach to client resource management #11

Merged
merged 3 commits into from
Feb 17, 2025
Merged

Conversation

sgbalogh
Copy link
Member

  • Builder pattern for clients
  • Clients implement AutoClosable but only shutdown owned resources (executor, grpc channel)
    • resource is owned if it was constructed via a default by the builder
  • Clients no longer create each other (no more account -> basin -> stream flow, as it complicates the ownership semantics)
  • ManagedChannelFactory, to expose simple util for constructing a channel given an s2 config
  • Wrapper on ManagedChannel that implements AutoClosable (only now can I see @ASRagab's wisdom in the original ChannelManager 😛, whoops )
  • ManagedReadSession and ManagedAppendSession also now impl AutoClosable
    • they dont close executor/channel, but do attempt to clean up outstanding futures, so as to avoid situations where the executor in which they are running is shutdown first

I think this is a more reasonable approach to dealing with these resources?

    try (final var executor = new ScheduledThreadPoolExecutor(1);
        final var channel = ManagedChannelFactory.forBasinOrStreamService(config, basinName)) {

      // share executor and channel between two stream clients:
      final var streamClient1 =
          StreamClient.newBuilder(config, basinName, "my-stream-1")
              .withExecutor(executor)
              .withChannel(channel)
              .build();

      final var streamClient2 =
          StreamClient.newBuilder(config, basinName, "my-stream-2")
              .withExecutor(executor)
              .withChannel(channel)
              .build();
  }

@@ -52,6 +52,6 @@ tasks.test {
java {
withSourcesJar()
toolchain {
languageVersion.set(JavaLanguageVersion.of(17))
languageVersion.set(JavaLanguageVersion.of(19))
Copy link
Member Author

@sgbalogh sgbalogh Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scheduled executor only impl's AutoClosable starting in 19, so the bump is required (just for the demo, not SDK)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@ASRagab
Copy link
Collaborator

ASRagab commented Feb 13, 2025

@sgbalogh yeah, I think I like this way better. I think I got bogged down in closing the previous channel and managing all the edge cases, this is much cleaner, also more idiomatic I think for these kinds of clients.

@sgbalogh sgbalogh marked this pull request as ready for review February 17, 2025 23:11
@sgbalogh sgbalogh merged commit a352918 into main Feb 17, 2025
2 checks passed
@sgbalogh sgbalogh deleted the channel-manager branch February 17, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants