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

Abstract ManagedChannel Creation #195

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lewisheadden
Copy link

While using this project, I found myself wanting to customize the ManagedChannel that was created. While I saw there was an option to override buildChannel, I thought it might be nicer to provide that as a dependency to Tiller instead.

As such, I created a ManagedChannelFactory interface which is used to create a ManagedChannel from a LocalPortForward. I also provide the default implementation (DefaultManagedChannelFactory) that migrates existing logic from the current buildChannel method. It also allows the provision of a ManagedChannelConfigurer which is passed the ManagedChannelBuilder and is able to manipulate it before it is finally built.

This allows users to do the following if they simply want to configure settings on the ManagedChannel instead of having to subclass Tiller.

final Tiller tiller = new Tiller(client, new DefaultManagedChannelFactory((builder) -> {
    builder
        .keepAliveTime(90L, TimeUnit.SECONDS)
        .keepAliveTimeout(30L, TimeUnit.SECONDS);
}));

Let me know if I've not explained anything well, or if you have any concerns or changes you'd like.

@ljnelson ljnelson self-assigned this Feb 4, 2019
@ljnelson
Copy link
Member

ljnelson commented Feb 4, 2019

Hello; thanks for using microBean Helm.

I like the general idea here; since what you want to do is not prohibited currently, I'd like to let this sit for a bit so I can chew on it.

This project is slightly unique in that one of my design goals/constraints is to expose as tiny an API surface area as possible, as it's just me on nights and weekends. Perhaps what you want to do is best served by simply accepting something like a Supplier<ManagedChannel>, and we just leave it at that.

@lewisheadden
Copy link
Author

@ljnelson Totally understood! I was trying to keep it to one component but the dependency wiring into Tiller seemed a little weird.

Take your time and let me know what you decide! Thanks for your hard work!

@ljnelson
Copy link
Member

I'm going to start by adding the ability to take in a Function<? super LocalPortForward, ? extends ManagedChannel> at construction time. After that, we can explore (through concrete usage, I hope) how painful it is to supply such a Function, and introduce, perhaps, additional implementations of it. Watch this space!

@ljnelson ljnelson added the in progress In progress label Feb 14, 2019
ljnelson added a commit that referenced this pull request Feb 14, 2019
@ljnelson
Copy link
Member

@lewisheadden I have deployed microbean-helm version 2.12.3.0.0.1-SNAPSHOT to the Sonatype snapshots repository. I would appreciate any testing you might be able to perform.

@lewisheadden
Copy link
Author

@ljnelson I recently left the job that this PR was related to and am taking some time off between roles but @jsvk is still working on this and might be able to give you some more timely insight.

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

Successfully merging this pull request may close these issues.

2 participants