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

Change behavior handling of consumer/supplier in builder #512

Closed
segabriel opened this issue Jan 31, 2019 · 7 comments
Closed

Change behavior handling of consumer/supplier in builder #512

segabriel opened this issue Jan 31, 2019 · 7 comments

Comments

@segabriel
Copy link
Member

for now, I lose the previous settings when I invoke the options setter several times:

      Microservices.builder()
            .discovery(options -> options.seeds(seed.discovery().address()))
            .discovery(options -> options.port(123))
            .discovery(options -> options.memberHost("member"))

I'd like to propose making it in the Microservices like this:

public Builder discovery(Consumer<ServiceDiscoveryConfig.Builder> discoveryOptions) {
     if (this.discoveryOptions != null) {
      this.discoveryOptions = this.discoveryOptions.andThen(discoveryOptions);
    } else {
      this.discoveryOptions = discoveryOptions;
    }
...
@ronenhamias
Copy link
Member

this is very verbose can you explain the value and what it try to solve?

@segabriel
Copy link
Member Author

@ronenhamias currently, we just override options when we invoke them again. For example, if I set some seed address firstly and then I want to add members and to do it I need to specify the seed again and the members.

@segabriel
Copy link
Member Author

And java Consumer and Supplier allow do it with andThen or thenApply

@ronenhamias
Copy link
Member

can you provide an example that demonstrate a problem or uscase that have issue and how it can be solved?

@segabriel
Copy link
Member Author

@ronenhamias What do you expect from the next sample?

Microservices.builder()
            .discovery(options -> options.host("177.17.0.1"))
            .discovery(options -> options.port(8888))

The current behavior will try to connect to localhost:8888 instead of 177.17.0.1:8888.

@artem-v
Copy link
Contributor

artem-v commented Mar 6, 2019

@segabriel

As you know I worked to fix this behavior just recently. Go ahead and verify you concerns with latest code.

@segabriel
Copy link
Member Author

@artem-v
I have revised it after your changes, and your changes fixed the mentioned problem (we always create a new ScalecubeServiceDiscovery instance and apply previous and new options).

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

No branches or pull requests

3 participants